[FFmpeg-devel] libavcodec: fix field_order labelling

Submitted by Dave Rice on Aug. 12, 2017, 4:47 p.m.

Details

Message ID F4FE4E00-8A8E-40A0-9421-97020A7D6E54@dericed.com
State New
Headers show

Commit Message

Dave Rice Aug. 12, 2017, 4:47 p.m.
Hello all,
This issue originated in this thread https://github.com/amiaopensource/vrecord/issues/170. On Field Order, in the QuickTime specification at https://developer.apple.com/library/content/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html (and similarly in the Matroska specification which adopted similar language) it states the following meanings for field order values:

> 9 – B is displayed earliest, T is stored first in the file. 14 – T is displayed earliest, B is stored first in the file.

 This definition is contradicted by other Apple documentation such as https://developer.apple.com/library/content/technotes/tn2162/_index.html#//apple_ref/doc/uid/DTS40013070-CH1-TNTAG10-THE__FIEL__IMAGEDESCRIPTION_EXTENSION__FIELD_FRAME_INFORMATION. 

An Apple engineer confirmed that the QuickTime specification’s definitions for those Field Order values is wrong and does not match Apple’s (of FFmpeg’s) practice, see https://github.com/amiaopensource/vrecord/issues/170#issuecomment-321937668.

However I think that some of the commenting in ffmpeg is based upon the inaccurate definitions from Apple. For instance, in that thread David Singer confirms:

> Ah, not quite. 1 and 6 are indeed 'planar' (all of one field before all of the other). They don't concern us. Both 9 and 14 are stored in spatial order (i.e. you could do terrible de-interlacing by simply displaying the buffer as a frame), and the 9 or 14 value tells you which field is to be displayed first.
> 
> 9 – T is earlier than B. 14 – B is earlier than T

mov.c associates AV_FIELD_TB with 9 and AV_FIELD_BT with 14 (similar associations in matroska.h), but avcodec.h states:

> AV_FIELD_TB,          //< Top coded first, bottom displayed first
> AV_FIELD_BT,          //< Bottom coded first, bottom displayed first

IMHO in both cases of AV_FIELD_TB and AV_FIELD_BT the coding should be referred as interleaved rather than ‘bottom coded first’ or ‘top coded first’. In the case of AV_FIELD_TT and AV_FIELD_BB the fields are stored as planar images where storage order is notable, but with TB and BT the fields are interleaved.

Also utils.c associates these field order values with the following labels:

> AV_FIELD_TB  -> "top coded first (swapped)";
> AV_FIELD_BT -> "bottom coded first (swapped)";

From my reading, I infer that "top coded first (swapped)” means "top coded first, bottom displayed first”; however in practice from files generated by QuickTime and FFmpeg files with a value of TB have the top field displayed first, so I think the labels are swapped. In the patch below I suggest using “top first (interleaved)” for TB and “bottom first (interleaved)” for BT.

Comments?


From de871b3fa891fa0ae6856461c1f8305cc889cde7 Mon Sep 17 00:00:00 2001
From: Dave Rice <dave@dericed.com>
Date: Sat, 12 Aug 2017 12:30:43 -0400
Subject: [PATCH] libavcodec: fix field_order labelling

---
 libavcodec/avcodec.h | 4 ++--
 libavcodec/utils.c   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Alex Converse Aug. 16, 2017, 3:54 a.m.
On Sat, Aug 12, 2017 at 9:47 AM, Dave Rice <dave@dericed.com> wrote:
>
> Hello all,
> This issue originated in this thread https://github.com/amiaopensource/vrecord/issues/170. On Field Order, in the QuickTime specification at https://developer.apple.com/library/content/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html (and similarly in the Matroska specification which adopted similar language) it states the following meanings for field order values:
>
> > 9 – B is displayed earliest, T is stored first in the file. 14 – T is displayed earliest, B is stored first in the file.
>
>  This definition is contradicted by other Apple documentation such as https://developer.apple.com/library/content/technotes/tn2162/_index.html#//apple_ref/doc/uid/DTS40013070-CH1-TNTAG10-THE__FIEL__IMAGEDESCRIPTION_EXTENSION__FIELD_FRAME_INFORMATION.
>
> An Apple engineer confirmed that the QuickTime specification’s definitions for those Field Order values is wrong and does not match Apple’s (of FFmpeg’s) practice, see https://github.com/amiaopensource/vrecord/issues/170#issuecomment-321937668.
>
> However I think that some of the commenting in ffmpeg is based upon the inaccurate definitions from Apple. For instance, in that thread David Singer confirms:
>
> > Ah, not quite. 1 and 6 are indeed 'planar' (all of one field before all of the other). They don't concern us. Both 9 and 14 are stored in spatial order (i.e. you could do terrible de-interlacing by simply displaying the buffer as a frame), and the 9 or 14 value tells you which field is to be displayed first.
> >
> > 9 – T is earlier than B. 14 – B is earlier than T
>
> mov.c associates AV_FIELD_TB with 9 and AV_FIELD_BT with 14 (similar associations in matroska.h), but avcodec.h states:
>
> > AV_FIELD_TB,          //< Top coded first, bottom displayed first
> > AV_FIELD_BT,          //< Bottom coded first, bottom displayed first
>
> IMHO in both cases of AV_FIELD_TB and AV_FIELD_BT the coding should be referred as interleaved rather than ‘bottom coded first’ or ‘top coded first’. In the case of AV_FIELD_TT and AV_FIELD_BB the fields are stored as planar images where storage order is notable, but with TB and BT the fields are interleaved.
>
> Also utils.c associates these field order values with the following labels:
>
> > AV_FIELD_TB  -> "top coded first (swapped)";
> > AV_FIELD_BT -> "bottom coded first (swapped)";
>
> From my reading, I infer that "top coded first (swapped)” means "top coded first, bottom displayed first”; however in practice from files generated by QuickTime and FFmpeg files with a value of TB have the top field displayed first, so I think the labels are swapped. In the patch below I suggest using “top first (interleaved)” for TB and “bottom first (interleaved)” for BT.
>
> Comments?
>

Icefloe019 agrees with your changes:
http://mirror.informatimago.com/next/developer.apple.com/quicktime/icefloe/dispatch019.html#fiel

They seems reasonable to me. when I made this change originally my
primary concern was getting the field info out of extradata.
Marton Balint Aug. 27, 2017, 5:13 p.m.
On Sat, 12 Aug 2017, Dave Rice wrote:

[..]

> Also utils.c associates these field order values with the following labels:
>
>> AV_FIELD_TB  -> "top coded first (swapped)";
>> AV_FIELD_BT -> "bottom coded first (swapped)";
>
> From my reading, I infer that "top coded first (swapped)” means "top 
> coded first, bottom displayed first”; however in practice from files 
> generated by QuickTime and FFmpeg files with a value of TB have the top 
> field displayed first, so I think the labels are swapped. In the patch 
> below I suggest using “top first (interleaved)” for TB and “bottom first 
> (interleaved)” for BT.
>
> Comments?
>
>
> From de871b3fa891fa0ae6856461c1f8305cc889cde7 Mon Sep 17 00:00:00 2001
> From: Dave Rice <dave@dericed.com>
> Date: Sat, 12 Aug 2017 12:30:43 -0400
> Subject: [PATCH] libavcodec: fix field_order labelling
>
> ---
> libavcodec/avcodec.h | 4 ++--
> libavcodec/utils.c   | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index c594993766..37c39072b3 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1726,8 +1726,8 @@ enum AVFieldOrder {
>     AV_FIELD_PROGRESSIVE,
>     AV_FIELD_TT,          //< Top coded_first, top displayed first
>     AV_FIELD_BB,          //< Bottom coded first, bottom displayed first
> -    AV_FIELD_TB,          //< Top coded first, bottom displayed first
> -    AV_FIELD_BT,          //< Bottom coded first, top displayed first
> +    AV_FIELD_TB,          //< Interleaved coding, top displayed first
> +    AV_FIELD_BT,          //< Interleaved coding, bottom displayed first
> };

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.

Regards,
Marton
Tobias Rapp Aug. 29, 2017, 1:16 p.m.
On 27.08.2017 19:13, Marton Balint wrote:
> 
> 
> On Sat, 12 Aug 2017, Dave Rice wrote:
> 
> [..]
> 
>> Also utils.c associates these field order values with the following 
>> labels:
>>
>>> AV_FIELD_TB  -> "top coded first (swapped)";
>>> AV_FIELD_BT -> "bottom coded first (swapped)";
>>
>> From my reading, I infer that "top coded first (swapped)” means "top 
>> coded first, bottom displayed first”; however in practice from files 
>> generated by QuickTime and FFmpeg files with a value of TB have the 
>> top field displayed first, so I think the labels are swapped. In the 
>> patch below I suggest using “top first (interleaved)” for TB and 
>> “bottom first (interleaved)” for BT.
>>
>> Comments?
>>
>>
>> From de871b3fa891fa0ae6856461c1f8305cc889cde7 Mon Sep 17 00:00:00 2001
>> From: Dave Rice <dave@dericed.com>
>> Date: Sat, 12 Aug 2017 12:30:43 -0400
>> Subject: [PATCH] libavcodec: fix field_order labelling
>>
>> ---
>> libavcodec/avcodec.h | 4 ++--
>> libavcodec/utils.c   | 4 ++--
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index c594993766..37c39072b3 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -1726,8 +1726,8 @@ enum AVFieldOrder {
>>     AV_FIELD_PROGRESSIVE,
>>     AV_FIELD_TT,          //< Top coded_first, top displayed first
>>     AV_FIELD_BB,          //< Bottom coded first, bottom displayed first
>> -    AV_FIELD_TB,          //< Top coded first, bottom displayed first
>> -    AV_FIELD_BT,          //< Bottom coded first, top displayed first
>> +    AV_FIELD_TB,          //< Interleaved coding, top displayed first
>> +    AV_FIELD_BT,          //< Interleaved coding, bottom displayed first
>> };
> 
> 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.

Regards,
Tobias
Jerome Martinez Sept. 2, 2017, 8:41 p.m.
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

Patch hide | download patch | download mbox

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index c594993766..37c39072b3 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -1726,8 +1726,8 @@  enum AVFieldOrder {
     AV_FIELD_PROGRESSIVE,
     AV_FIELD_TT,          //< Top coded_first, top displayed first
     AV_FIELD_BB,          //< Bottom coded first, bottom displayed first
-    AV_FIELD_TB,          //< Top coded first, bottom displayed first
-    AV_FIELD_BT,          //< Bottom coded first, top displayed first
+    AV_FIELD_TB,          //< Interleaved coding, top displayed first
+    AV_FIELD_BT,          //< Interleaved coding, bottom displayed first
 };
 
 /**
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 1336e921c9..926c964846 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -1406,9 +1406,9 @@  void avcodec_string(char *buf, int buf_size, AVCodecContext *enc, int encode)
                 else if (enc->field_order == AV_FIELD_BB)
                     field_order = "bottom first";
                 else if (enc->field_order == AV_FIELD_TB)
-                    field_order = "top coded first (swapped)";
+                    field_order = "top first (interleaved)";
                 else if (enc->field_order == AV_FIELD_BT)
-                    field_order = "bottom coded first (swapped)";
+                    field_order = "bottom first (interleaved)";
 
                 av_strlcatf(detail, sizeof(detail), "%s, ", field_order);
             }