[FFmpeg-devel] [PATCH 1/4] mxfdec: Index table based seeking
georg.lippitsch at gmx.at
Thu Sep 29 11:13:42 CEST 2011
Am 29.09.2011, 10:04 Uhr, schrieb Tomas Härdin <tomas.hardin at codemill.se>:
>> + if (current == 0 || segment->stream_offset_entries[current-1]
>> != stream_offset_entry)
> Might want a comment above this "if" explaining why this is needed.
> Something like /* handle Avid index tables with duplicate entries */
Indeed. As you have seen I was quite lazy with comments also in the
earlier patches, but when reviewing them now some weeks later I find that
annoying. I really should care more about comments and documentation ...
> /* account for Avid's index tables having an extra entry for the size */
> segment->nb_unique_index_entries = current ==
> segment->nb_index_entries ? current : current - 1;
Actually the n+1 doesn't hurt because in the later parsing, we only use
index_duration (not nb_index_entries) for the loop. But if a program would
not add the +1, one entry is missing, so I'd prefer setting it to current
>> seg = (MXFIndexTableSegment *)mxf->metadata_sets[i];
>> - for (j = 0; j < seg->nb_index_entries; j++)
>> - av_freep(&seg->slice_offset_entries[j]);
>> + if (seg->slice_count)
>> + for (j = 0; j < seg->nb_index_entries; j++)
>> + av_freep(&seg->slice_offset_entries[j]);
> Unrelated? Also, aren't all of these NULL if !slice_count and thus
> av_freep() is OK?
No. This caused a segfault. seg->slice_offset_entries itself is not
allocated if seg->slice_count is 0, and therefore cannot be accessed with
Updated patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 4690 bytes
Desc: not available
More information about the ffmpeg-devel