[FFmpeg-devel] [PATCH] libavcodec: fix field_order labelling

[Jerome Martinez]

Le 29/08/2017 à 15:16, Tobias Rapp a écrit :
> On 27.08.2017 19:13, Marton Balint wrote:
>>
>> I agree that a lot of stuff in the codebase is consistent with the 
>> updated descriptions. However, as far as I see libavformat/mxfdec.c 
>> seems to follow the existing docs, so I think that needs changing as 
>> well.
>
> In mxfdec.c the value of field_order is constructed from VideoLineMap 
> and FieldDominance. The VideoLineMap property indicates coded field 
> order and the FieldDominance property indicated whether display field 
> order is flipped compared to the coded order or not.
>
> So yes, mxfdec.c is following the current documentation of 
> AVFieldOrder and applying the patch would conflict with the MXF specs, 
> AFAICS.
>
> CC-ing Jérôme Martinez as he has much more experience with MXF 
> real-world file variations.

I think that the source of the issue is that different meanings were 
mapped to the same flag.
My understanding is that we have 3 items (instead of 2) for interlaced 
content:
- Store method (separate fields or interleaved fields)
- Store order (Top or Bottom field first)
- Display order (Top or Bottom field first)
I understand that MOV (and MKV) have store method and store order, then 
display order is always store order. The patch would fix the issue 
between MOV/MKV specs and FFmpeg analysis.

For MXF, looks like we have also display order which may be not store 
order, with "field dominance", from specs:
"A value of 1 shall indicate that the first field is first in temporal 
order. A value of 2 shall indicate that the second field is the first in 
temporal order."
Reading FFmpeg code, I understand something similar to Marton 
understanding and the patch can not be applied as is, there is a need to 
separate cases, but this implies that FFmpeg needs to handle 3 "flags" 
instead of 2, then there is a need to check all formats that the mapping 
is correct (e.g. I have no idea about the MJPEG stuff, looks like complex).

I suggest a complex change, but I don't see another method: more AV_FIELD_*!
AV_FIELD_UNKNOWN,
AV_FIELD_PROGRESSIVE,
AV_FIELD_STT: separate fields, top coded first, top displayed first
AV_FIELD_SBB: separate fields, bottom coded first, bottom displayed first
AV_FIELD_STB: separate fields, top coded first, bottom displayed first
AV_FIELD_SBT: separate fields, bottom coded first, top displayed first
AV_FIELD_ITT: interleaved fields, top coded first, top displayed first
AV_FIELD_IBB: interleaved fields, bottom coded first, bottom displayed first
AV_FIELD_ITB: interleaved fields, top coded first, bottom displayed first
AV_FIELD_IBT: interleaved fields, bottom coded first, top displayed first

For MXF, field_topness = (descriptor->video_line_map[0] + 
descriptor->video_line_map[1]) % 2
For MOV, it includes MKV (same spec) and is 'fiel' atom value

AV_FIELD_STT: MOV 1, MXF frame_layout=SINGLE_FIELD field_topness=1 
field_dominance=1
AV_FIELD_SBB: MOV 6, MXF frame_layout=SINGLE_FIELD field_topness=0 
field_dominance=1
AV_FIELD_STB: MXF frame_layout=SINGLE_FIELD field_topness=1 
field_dominance=0
AV_FIELD_SBT: MXF frame_layout=SINGLE_FIELD field_topness=0 
field_dominance=0
AV_FIELD_ITT: MOV 9, MXF frame_layout=MIXED_FIELDS field_topness=1 
field_dominance=1
AV_FIELD_IBB: MOV 14, MXF frame_layout=MIXED_FIELDS field_topness=0 
field_dominance=1
AV_FIELD_STB: MXF frame_layout=MIXED_FIELDS field_topness=1 
field_dominance=0
AV_FIELD_SBT: MXF frame_layout=MIXED_FIELDS field_topness=0 
field_dominance=0

In other  words, for MXF:
- frame_layout is for 1st letter (new!)
- video_line_map is for 2nd letter (was 1st letter)
- field_dominance is for 3rd letter (was 2nd letter)
and for MOV/MKV:
- <8 or >=8 for 1st letter (current)
- &0x7 for 2nd letter (current)
- 3rd letter is a copy of 2nd letter (no "dominance" possible in MOV/MKV)

Notes while reading MXF parsing in FFmpeg:
MIXED_FIELDS: there is a "break", but IMO algo for field_order should apply
SEGMENTED_FRAME: specs say "the value of field dominance has no meaning 
and should not be present." but if FieldDominance is in the file, 
FieldDominance is used qlso for SEGMENTED_FRAME (no "break"). height is 
also multiplied by 2 but specs say that height is already the frame 
height. But I have no file for testing.

Note while reading e.g. FFV1 encoder code:
I see:
         if (avctx->field_order == AV_FIELD_TT || avctx->field_order == 
AV_FIELD_TB)
             p->top_field_first = 1;
if we have 8 cases, such kind of check maybe be huge (test on 4 values), 
maybe flags are more relevant rather that enums but it breaks more 
things I guess. Additionally I understand (but I am not a FFmpeg expert, 
confirmation?) that we lose info about which field should be displayed 
first (only which field is coded first is stored), and the decoder maps 
to top_field_first value only (no mapping to field_order). I wonder if 
we should not check display order instead of store order.

Note for other formats:
I don't know enough about other formats, e.g. v210 is interleaved of 
separate fields? each formats would have to be checked.


I would like to have this patch integrated because currently MOV and MKV 
are wrongly interpreted, but I don't find a solution without expanding 
the AV_FIELD_*, does anyone has a quicker/intermediate solution?

Jérôme