[FFmpeg-devel] [PATCH 4/4] lavf/mov: Add support for edit list parsing.

[Clément Bœsch]

On Wed, Aug 10, 2016 at 07:35:00PM -0700, Sasi Inguva wrote:
[...]
> +/**
> + * Get ith edit list entry (media time, duration).
> + */
> +static int get_edit_list_entry(MOVStreamContext *msc,

msc should be marked as const

> +                               unsigned int edit_list_index,
> +                               int64_t *edit_list_media_time,
> +                               int64_t *edit_list_duration,
> +                               int64_t global_timescale)
> +{
> +    if (edit_list_index == msc->elst_count) {
> +        return 0;
> +    }
> +    *edit_list_media_time = msc->elst_data[edit_list_index].time;
> +    *edit_list_duration = msc->elst_data[edit_list_index].duration;

> +    /* duration is in global timescale units;convert to msc timescale */
> +    *edit_list_duration *= msc->time_scale;
> +    *edit_list_duration /= global_timescale;

it looks like this may overflow; you should use one of the av_rescale*
function

> +    return 1;
> +}
> +
> +/**
> + * Find the closest previous keyframe to the timestamp, in e_old index
> + * entries.
> + * Returns the index of the entry in st->index_entries if successful,
> + * else returns -1.
> + */
> +static int64_t find_prev_closest_keyframe_index(AVStream *st,
> +                                                AVIndexEntry *e_old,
> +                                                int nb_old,
> +                                                int64_t timestamp,
> +                                                int flag)
> +{
> +    AVIndexEntry *e_keep = st->index_entries;
> +    int nb_keep = st->nb_index_entries;
> +    int64_t found = -1;
> +
> +    st->index_entries = e_old;
> +    st->nb_index_entries = nb_old;
> +    found = av_index_search_timestamp(st, timestamp, flag | AVSEEK_FLAG_BACKWARD);
> +

> +    /* restore AVStream  state*/

nit: misplaced space

> +    st->index_entries = e_keep;
> +    st->nb_index_entries = nb_keep;
> +    return found;
> +}
> +
> +/**
> + * Add index entry with the given values, to the end of st->index_entries.
> + * Returns the new size st->index_entries if successful, else returns -1.
> + */
> +static int64_t add_index_entry(AVStream *st, int64_t pos, int64_t timestamp,
> +                               int size, int distance, int flags)
> +{
> +    AVIndexEntry *entries, *ie;
> +    int64_t index = -1;
> +    const size_t min_size_needed = (st->nb_index_entries + 1) * sizeof(AVIndexEntry);
> +    const size_t requested_size =
> +        min_size_needed > st->index_entries_allocated_size ?
> +        FFMAX(min_size_needed, 2 * st->index_entries_allocated_size) :
> +        min_size_needed;
> +
> +    if((unsigned)st->nb_index_entries + 1 >= UINT_MAX / sizeof(AVIndexEntry))
> +        return -1;
> +
> +    entries = av_fast_realloc(st->index_entries,
> +                              &st->index_entries_allocated_size,
> +                              requested_size);
> +    if(!entries)
> +        return -1;
> +
> +    st->index_entries= entries;
> +
> +    index= st->nb_index_entries++;
> +    ie= &entries[index];

> +    assert(!index || ie[-1].timestamp <= timestamp);

we use av_assert*

> +
> +    ie->pos = pos;
> +    ie->timestamp = timestamp;
> +    ie->min_distance= distance;
> +    ie->size= size;
> +    ie->flags = flags;
> +    return index;
> +}

This function somehow looks like a copy of an old version of
ff_add_index_entry() (I'm hinted by the coding style); it would be nice to
keep differences as small as possible, and maybe hint about similarity and
the reason why it differs

[...]
> diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
> new file mode 100644
> index 0000000..5b3a187
> --- /dev/null
> +++ b/tests/fate/mov.mak
> @@ -0,0 +1,28 @@

> +FATE_MOV = fate-mov-3elist \
> +	   fate-mov-3elist-1ctts \
> +	   fate-mov-1elist-1ctts \
> +	   fate-mov-1elist-noctts \
> +	   fate-mov-elist-starts-ctts-2ndsample \
> +	   fate-mov-1elist-ends-last-bframe \
> +	   fate-mov-2elist-elist1-ends-bframe
> +

You have tabs here that should be replaced with spaces

> +FATE_SAMPLES_AVCONV += $(FATE_MOV)
> +
> +fate-mov: $(FATE_MOV)
> +
> +# Make sure we handle edit lists correctly in normal cases.
> +fate-mov-1elist-noctts: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-1elist-noctts.mov
> +fate-mov-1elist-1ctts: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-1elist-1ctts.mov
> +fate-mov-3elist: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-3elist.mov
> +fate-mov-3elist-1ctts: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-3elist-1ctts.mov
> +
> +# Makes sure that the CTTS is also modified when we fix avindex in mov.c while parsing edit lists.
> +fate-mov-elist-starts-ctts-2ndsample: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-elist-starts-ctts-2ndsample.mov
> +
> +# Makes sure that we handle edit lists ending on a B-frame correctly.
> +# The last frame in decoding order which is B-frame should be output, but the last but-one P-frame shouldn't be
> +# output.
> +fate-mov-1elist-ends-last-bframe: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-1elist-ends-last-bframe.mov
> +
> +# Makes sure that we handle timestamps of packets in case of multiple edit lists with one of them ending on a B-frame correctly.
> +fate-mov-2elist-elist1-ends-bframe: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-2elist-elist1-ends-bframe.mov

is there any behaviour changes when doing stream copy btw?

> \ No newline at end of file

i think git is trying to tell you something here

[...]

i still have to test a few more samples (which i do not have access right
now). i'll report about those soon, but so far it seems to be working for
the samples that were causing me issues in the past.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160814/7586bfc6/attachment.sig>