[FFmpeg-devel] [PATCH 1/4] mxfdec: Index table based seeking

[Georg Lippitsch]

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  
directly.

>>              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  
seg->slice_offset_entries[j]

Updated patch attached.

Georg
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mxfdec-Avid-XDCAM.patch
Type: text/x-patch
Size: 4690 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110929/e08ba52a/attachment.bin>