[FFmpeg-devel] [PATCH] Clip-wrapped MXF support (new attempt)

[Baptiste Coudurier]

Hi,

On 04/14/2010 06:57 AM, Maksym Veremeyenko wrote:
> Hi,
>
> Third attempt to implement Clip-wrapped MXF support for ffmpeg. It makes
> possible to decode/demux P2's files.
>
> Patches:
>
> *0001-revert-MXFCodecUL-struct-modification-from-r11567.patch*
> *0002-partially-revert-essence_container-from-r11567.patch*
> reverts some changes from r11567 that simplify wrapping detection but
> work wrong some items.

That may be right. Can you please add a new struct only for the 
container uls ? All the codecs uls are not wrapping scheme related.

> *0003-make-key-output-in-RP-224.10-form.patch*
> on debug output PRINT_KEY in a form represented in
> /RP224v10-publication-20081215/ with dots as hex codes separator
>
> *0004-add-new-essence-container-uls-and-wrapping-methods.patch*
> adds known (or maybe possible in my case) container uls with proper wrap
> method

Will be added later when code actually use them.
Also, the code should match the codec first then try the container, to 
avoid avoid huge tables.

> *0005-extend-MXFIndexTableSegment.patch*
> extends *MXFIndexTableSegment* struct members and appropriate read function

See comments below.

> *0006-clip-wrapped-support-added.patch*
> actually adds clip-wrapped code.

Can you please isolate the clip wrapping reading code in another function ?
Also, there will be interleaving problems when done this way.
This will extract one whole track then the other track.
No player can reasonably buffer that much data.

>
> [...]
 >
>
>   typedef struct {
> @@ -503,12 +509,32 @@ static int mxf_read_source_package(MXFPackage *package, ByteIOContext *pb, int t
>   static int mxf_read_index_table_segment(MXFIndexTableSegment *segment, ByteIOContext *pb, int tag)
>   {
>       switch(tag) {
> -    case 0x3F05: dprintf(NULL, "EditUnitByteCount %d\n", get_be32(pb)); break;
> -    case 0x3F06: dprintf(NULL, "IndexSID %d\n", get_be32(pb)); break;
> -    case 0x3F07: dprintf(NULL, "BodySID %d\n", get_be32(pb)); break;
> -    case 0x3F0B: dprintf(NULL, "IndexEditRate %d/%d\n", get_be32(pb), get_be32(pb)); break;
> -    case 0x3F0C: dprintf(NULL, "IndexStartPosition %lld\n", get_be64(pb)); break;
> -    case 0x3F0D: dprintf(NULL, "IndexDuration %lld\n", get_be64(pb)); break;
> +    case 0x3F05:
> +        segment->edit_unit_byte_count = get_be32(pb);
> +        av_log(NULL, AV_LOG_DEBUG, "EditUnitByteCount %d\n", segment->edit_unit_byte_count);
> +        break;
> +    case 0x3F06:
> +        segment->index_sid = get_be32(pb);
> +        av_log(NULL, AV_LOG_DEBUG, "IndexSID %d\n", segment->index_sid);
> +        break;
> +    case 0x3F07:
> +        segment->body_sid = get_be32(pb);
> +        av_log(NULL, AV_LOG_DEBUG, "BodySID %d\n", segment->body_sid);
> +        break;
> +    case 0x3F0B:
> +        segment->index_edit_rate.num = get_be32(pb);
> +        segment->index_edit_rate.den = get_be32(pb);
> +        av_log(NULL, AV_LOG_DEBUG, "IndexEditRate %d/%d\n", segment->index_edit_rate.num,
> +               segment->index_edit_rate.num);
> +        break;
> +    case 0x3F0C:
> +        segment->index_start_position = get_be64(pb);
> +        av_log(NULL, AV_LOG_DEBUG, "IndexStartPosition %lld\n", segment->index_start_position);
> +        break;
> +    case 0x3F0D:
> +        segment->index_duration = get_be64(pb);
> +        av_log(NULL, AV_LOG_DEBUG, "IndexDuration %lld\n", segment->index_duration);
> +        break;

Logging without context by default is not really ok. Please keep it 
dprintf for now, or extends func to take AVFormatContext as parameter.

[...]

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org