[FFmpeg-devel] ffmpeg: fix setting field_order during muxing

Submitted by James Almer on Aug. 10, 2017, 2:43 a.m.

Details

Message ID 20170810024336.7540-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Aug. 10, 2017, 2:43 a.m.
AVFrame.top_field_first doxy states

"If the content is interlaced, is top field displayed first."

And AVFieldOrder doxy defines:
    AV_FIELD_TB,          //< Top coded first, bottom displayed first
    AV_FIELD_BT,          //< Bottom coded first, top displayed first

Fixes ticket #6577

Signed-off-by: James Almer <jamrial@gmail.com>
---
 ffmpeg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Hendrik Leppkes Aug. 10, 2017, 8:21 a.m.
On Thu, Aug 10, 2017 at 4:43 AM, James Almer <jamrial@gmail.com> wrote:
> AVFrame.top_field_first doxy states
>
> "If the content is interlaced, is top field displayed first."
>
> And AVFieldOrder doxy defines:
>     AV_FIELD_TB,          //< Top coded first, bottom displayed first
>     AV_FIELD_BT,          //< Bottom coded first, top displayed first
>
> Fixes ticket #6577
>

Isn't top coded first in most codecs? So maybe it should use TT (top
coded and displayed first), and TB (top coded first, bottom displayed
first)?
I suppose that difference between coding order might be highly codec
dependent, though.

- Hendrik
James Almer Aug. 10, 2017, 4:29 p.m.
On 8/10/2017 5:21 AM, Hendrik Leppkes wrote:
> On Thu, Aug 10, 2017 at 4:43 AM, James Almer <jamrial@gmail.com> wrote:
>> AVFrame.top_field_first doxy states
>>
>> "If the content is interlaced, is top field displayed first."
>>
>> And AVFieldOrder doxy defines:
>>     AV_FIELD_TB,          //< Top coded first, bottom displayed first
>>     AV_FIELD_BT,          //< Bottom coded first, top displayed first
>>
>> Fixes ticket #6577
>>
> 
> Isn't top coded first in most codecs? So maybe it should use TT (top
> coded and displayed first), and TB (top coded first, bottom displayed
> first)?

I can make it TT/TB if that's preferred, but it would be as much of a
guess as it currently is.

> I suppose that difference between coding order might be highly codec
> dependent, though.

I don't know. ffmpeg.c right now is using TT/BB for mjpeg only, so it
has at least one codec specific case.

A quick grep shows codecs like h264, vc1 and canopus using TT/BB, and
ffv1 using TT/TB, so someone more familiar with this might want to take
a look at it and add some other cases as required, at least for codecs
supported in containers that care about the contents of
AVFormatContext.field_order (matroska, mov, etc).

> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Jerome Martinez Aug. 10, 2017, 9:18 p.m.
Le 10/08/2017 à 04:43, James Almer a écrit :
> AVFrame.top_field_first doxy states
>
> "If the content is interlaced, is top field displayed first."
>
> And AVFieldOrder doxy defines:
>      AV_FIELD_TB,          //< Top coded first, bottom displayed first
>      AV_FIELD_BT,          //< Bottom coded first, top displayed first
>
> Fixes ticket #6577

IMHO the subject is complex, and everyone should be in sync with the 
purpose of each mux_par->field_order value and understand impact on 
other players (sometimes the MOV metadata is the only one available as 
the codec has no info e.g. uncompressed or jp2k) before changing current 
behavior in a so general manner.
What I understood is that it is based on QuickTime specs (the mapping is 
1 to 1 from fiel atom) and TN2162 and that they are not easy to 
understand and maybe misleading : the feedback I got in the last years 
about the meaning of this atom are not really in sync with specs having 
stored vs displayed, and such change may have an impact on how other 
players handle fiel atom or other metadata in the way it is currently 
implemented by FFmpeg.
Dave Rice Aug. 10, 2017, 9:37 p.m.
> On Aug 10, 2017, at 5:18 PM, Jerome Martinez <Jerome@MediaArea.net> wrote:
> 
> Le 10/08/2017 à 04:43, James Almer a écrit :
>> AVFrame.top_field_first doxy states
>> 
>> "If the content is interlaced, is top field displayed first."
>> 
>> And AVFieldOrder doxy defines:
>>     AV_FIELD_TB,          //< Top coded first, bottom displayed first
>>     AV_FIELD_BT,          //< Bottom coded first, top displayed first
>> 
>> Fixes ticket #6577
> 
> IMHO the subject is complex, and everyone should be in sync with the purpose of each mux_par->field_order value and understand impact on other players (sometimes the MOV metadata is the only one available as the codec has no info e.g. uncompressed or jp2k) before changing current behavior in a so general manner.
> What I understood is that it is based on QuickTime specs (the mapping is 1 to 1 from fiel atom) and TN2162 and that they are not easy to understand and maybe misleading : the feedback I got in the last years about the meaning of this atom are not really in sync with specs having stored vs displayed, and such change may have an impact on how other players handle fiel atom or other metadata in the way it is currently implemented by FFmpeg.

I filed ticket #6577 but not feel cautious about this patch and consider that my initial report may have been based on some misunderstanding. As Jerome notes the MKV field order flag is derived from mov's fiel atom and there are two definitions for that:

https://developer.apple.com/library/content/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html (Table 4-2)

https://developer.apple.com/library/content/technotes/tn2162/_index.html#//apple_ref/doc/uid/DTS40013070-CH1-TNTAG10-THE__FIEL__IMAGEDESCRIPTION_EXTENSION__FIELD_FRAME_INFORMATION (Figure 4)

From testing we have found that ffmpeg's behavior here is matching some Apple mov muxers in the assignment of a field order value, so the patch may be unneeded or the reason may need more testing.

Dave Rice

Patch hide | download patch | download mbox

diff --git a/ffmpeg.c b/ffmpeg.c
index 888d19a647..a08db61ea3 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -1206,7 +1206,7 @@  static void do_video_out(OutputFile *of,
            avoid any copies. We support temporarily the older
            method. */
         if (in_picture->interlaced_frame)
-            mux_par->field_order = in_picture->top_field_first ? AV_FIELD_TB:AV_FIELD_BT;
+            mux_par->field_order = in_picture->top_field_first ? AV_FIELD_BT:AV_FIELD_TB;
         else
             mux_par->field_order = AV_FIELD_PROGRESSIVE;
         pkt.data   = (uint8_t *)in_picture;
@@ -1229,7 +1229,7 @@  static void do_video_out(OutputFile *of,
             if (enc->codec->id == AV_CODEC_ID_MJPEG)
                 mux_par->field_order = in_picture->top_field_first ? AV_FIELD_TT:AV_FIELD_BB;
             else
-                mux_par->field_order = in_picture->top_field_first ? AV_FIELD_TB:AV_FIELD_BT;
+                mux_par->field_order = in_picture->top_field_first ? AV_FIELD_BT:AV_FIELD_TB;
         } else
             mux_par->field_order = AV_FIELD_PROGRESSIVE;