[FFmpeg-devel] [PATCH 2/2] mxfdec: set audio packets pts
Tomas Härdin
tomas.hardin at codemill.se
Fri Sep 21 15:00:51 CEST 2012
On Thu, 2012-09-20 at 20:31 +0200, Matthieu Bouron wrote:
> Also fix playback of ntsc files.
> ---
> libavformat/mxfdec.c | 84 +++++++++++++++++++++++++++++++++++++++++++--
> tests/ref/fate/mxf-demux | 6 ++--
> tests/ref/lavf/mxf | 2 +-
> tests/ref/seek/lavf_mxf | 18 +++++-----
> tests/ref/seek/lavf_mxf_d10 | 30 ++++++++--------
> 5 files changed, 109 insertions(+), 31 deletions(-)
>
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 16b8c12..eb33b67 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -223,6 +223,8 @@ typedef struct {
> int nb_index_tables;
> MXFIndexTable *index_tables;
> int edit_units_per_packet; ///< how many edit units to read at a time (PCM, OPAtom)
> + AVRational *streams_time_bases;
> + uint64_t *streams_samples;
Why not put a simple AVRational and uint64_t in MXFTrack?
> } MXFContext;
>
> enum MXFWrappingScheme {
> @@ -1431,6 +1433,15 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
> ret = AVERROR(ENOMEM);
> goto fail_and_free;
> }
> +
> + mxf->streams_time_bases = av_realloc_f(mxf->streams_time_bases, mxf->fc->nb_streams, sizeof(*mxf->streams_time_bases));
> + mxf->streams_samples = av_realloc_f(mxf->streams_samples, mxf->fc->nb_streams, sizeof(*mxf->streams_samples));
> + if (!mxf->streams_time_bases || !mxf->streams_samples) {
> + av_log(mxf->fc, AV_LOG_ERROR, "could not allocate stream information\n");
> + ret = AVERROR(ENOMEM);
> + goto fail_and_free;
This will leak memory if mxf->streams_time_bases can be allocated but
mxf->streams_samples can't. Not a problem if you put these in MXFTrack
of course :)
> + }
> +
> st->id = source_track->track_id;
> st->priv_data = source_track;
> st->duration = component->duration;
> @@ -1438,6 +1449,8 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
> st->duration = AV_NOPTS_VALUE;
> st->start_time = component->start_position;
> avpriv_set_pts_info(st, 64, material_track->edit_rate.den, material_track->edit_rate.num);
> + mxf->streams_samples[mxf->fc->nb_streams - 1] = 0;
> + mxf->streams_time_bases[mxf->fc->nb_streams - 1] = av_inv_q(material_track->edit_rate);
>
> PRINT_KEY(mxf->fc, "data definition ul", source_track->sequence->data_definition_ul);
> codec_ul = mxf_get_codec_ul(ff_mxf_data_definition_uls, &source_track->sequence->data_definition_ul);
> @@ -1541,8 +1554,14 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
> st->codec->channels = descriptor->channels;
> st->codec->bits_per_coded_sample = descriptor->bits_per_sample;
>
> - if (descriptor->sample_rate.den > 0)
> + if (descriptor->sample_rate.den > 0) {
> + st->time_base = av_inv_q(descriptor->sample_rate);
Shouldn't you modify the call to avpriv_set_pts_info() instead?
> st->codec->sample_rate = descriptor->sample_rate.num / descriptor->sample_rate.den;
> + } else {
> + av_log(mxf->fc, AV_LOG_WARNING, "invalid sample rate (%d/%d) found for stream #%d, time base forced to 1/48000\n",
> + descriptor->sample_rate.num, descriptor->sample_rate.den, st->index);
> + st->time_base = (AVRational){1, 48000};
Seems relatively reasonable, apart from the avpriv_set_pts_info() thing.
> + }
>
> /* TODO: implement AV_CODEC_ID_RAWAUDIO */
> if (st->codec->codec_id == AV_CODEC_ID_PCM_S16LE) {
> @@ -1982,6 +2001,52 @@ static int64_t mxf_set_current_edit_unit(MXFContext *mxf, int64_t current_offset
> return next_ofs;
> }
>
> +static int mxf_compute_sample_count(MXFContext *mxf, int stream_index, uint64_t *sample_count)
> +{
> + int i, total = 0, size = 0;
> + AVRational time_base = mxf->streams_time_bases[stream_index];
> + const MXFSamplesPerFrame *spf;
> +
> + spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
> + if (!spf) {
> + av_log(mxf->fc, AV_LOG_ERROR, "unsupported codec time base %d/%d",
> + time_base.num, time_base.den);
> + return AVERROR(EINVAL);
This seems a bit harsh - could we fall back to the old code instead?
That or just compute samples_per_frames - nag only if the edit rate
doesn't divide sample_rate.
> + }
> +
> + while (spf->samples_per_frame[size]) {
> + total += spf->samples_per_frame[size];
> + size++;
> + }
> +
> + if (!size)
> + return AVERROR(EINVAL);
It seems like this can never happen - I'd av_assert() here (assuming I
understand correctly).
> + *sample_count = (mxf->current_edit_unit / size) * total;
> + for (i = 0; i < mxf->current_edit_unit % size; i++) {
> + *sample_count += spf->samples_per_frame[i];
> + }
> + return 0;
> +}
> +
> +static int mxf_set_audio_pts(MXFContext *mxf, AVCodecContext *codec, AVPacket *pkt)
> +{
> + int index = pkt->stream_index;
> + uint64_t current_sample_count = 0;
> + int ret = mxf_compute_sample_count(mxf, index, ¤t_sample_count);
> + if (ret < 0)
> + return ret;
> +
> + // read sample count and computed sample count max difference is 1 at most
> + if (fabs(current_sample_count - mxf->streams_samples[index]) > 1)
FFABS()
> + mxf->streams_samples[index] = current_sample_count;
This should only happen during seeking. You should always extrapolate
when demuxing straight ahead, else the packet durations (delta pts)
don't match pkt->size/(channels*bytes_per_sample).
> +
> + pkt->pts = mxf->streams_samples[index];
> + mxf->streams_samples[index] += pkt->size / (codec->channels * av_get_bits_per_sample(codec->codec_id) / 8);
Isn't there a function to get number of *bytes* per sample?
> + return 0;
> +}
> +
> static int mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt)
> {
> KLVPacket klv;
> @@ -2007,6 +2072,7 @@ static int mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt)
> int64_t next_ofs, next_klv;
> AVStream *st;
> MXFTrack *track;
> + AVCodecContext *codec;
>
> if (index < 0) {
> av_log(s, AV_LOG_ERROR, "error getting stream index %d\n", AV_RB32(klv.key+12));
> @@ -2045,7 +2111,8 @@ static int mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt)
> pkt->stream_index = index;
> pkt->pos = klv.offset;
>
> - if (s->streams[index]->codec->codec_type == AVMEDIA_TYPE_VIDEO && next_ofs >= 0) {
> + codec = s->streams[index]->codec;
> + if (codec->codec_type == AVMEDIA_TYPE_VIDEO && next_ofs >= 0) {
> /* mxf->current_edit_unit good - see if we have an index table to derive timestamps from */
> MXFIndexTable *t = &mxf->index_tables[0];
>
> @@ -2057,6 +2124,10 @@ static int mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt)
> * let utils.c figure out DTS since it can be < PTS if low_delay = 0 (Sony IMX30) */
> pkt->pts = mxf->current_edit_unit;
> }
> + } else if (codec->codec_type == AVMEDIA_TYPE_AUDIO && next_ofs >= 0) {
> + int ret = mxf_set_audio_pts(mxf, codec, pkt);
> + if (ret < 0)
> + return ret;
Looks reasonable. I assume you tested with OP1a.
> }
>
> /* seek for truncated packets */
> @@ -2114,13 +2185,18 @@ static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
> if ((size = av_get_packet(s->pb, pkt, size)) < 0)
> return size;
>
> + pkt->stream_index = 0;
> +
Why move this?
> if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO && t->ptses &&
> mxf->current_edit_unit >= 0 && mxf->current_edit_unit < t->nb_ptses) {
> pkt->dts = mxf->current_edit_unit + t->first_dts;
> pkt->pts = t->ptses[mxf->current_edit_unit];
> + } else if (st->codec->codec_type == AVMEDIA_TYPE_AUDIO) {
> + int ret = mxf_set_audio_pts(mxf, st->codec, pkt);
> + if (ret < 0)
> + return ret;
Did you test OPAtom?
Overall this looks very promising :)
/Tomas
More information about the ffmpeg-devel
mailing list