| Message ID | F4FE4E00-8A8E-40A0-9421-97020A7D6E54@dericed.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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); }