[FFmpeg-devel] [PATCH] avformat/yuv4mpegenc: simplify writing the header

[Jan Ekström]

On Thu, Sep 10, 2020 at 3:00 AM James Almer <jamrial at gmail.com> wrote:
>
> On 9/9/2020 7:54 PM, Michael Niedermayer wrote:
> > On Thu, Sep 03, 2020 at 03:55:04PM -0300, James Almer wrote:
> >> Actually write it in yuv4_write_header() instead of with the first
> >> packet.
> >>
> >> Signed-off-by: James Almer <jamrial at gmail.com>
> >> ---
> >>  libavformat/yuv4mpegenc.c | 35 ++++++++++++++---------------------
> >>  1 file changed, 14 insertions(+), 21 deletions(-)
> >
> > This changes the written header
> > for example:
> > ./ffmpeg  -i ~/tickets/2190/clip.yuv -bitexact testbad.y4m
> >
> > YUV4MPEG2 W720 H480 F30000:1001 Ip A10:11 C411 XYSCSS=411
> > vs
> > YUV4MPEG2 W720 H480 F30000:1001 Ib A10:11 C411 XYSCSS=411
>
> If I'm reading this right, ffmpeg.c changes the output AVCodecParameters
> struct (In this case, field_order) after calling avformat_write_header()
> but before writing a packet, which is wrong.
>
> We could revert this change (And the one by Andreas that came after it),
> but perhaps ffmpeg.c should be fixed to not violate the API instead.

My patch set that moves the encoder initialization later actually
enables moving this logic to before the encoder or the stream are
initialized, and fixes this difference.

Unfortunately, the resulting values which are now available for
general usage in header writing lead to various differences in FATE
tests (mostly tt switching tb) a la:

diff --git a/tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10
b/tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10
index 79ce1e2306..8f3f2e5265 100644
--- a/tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10
+++ b/tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10
@@ -78,5 +78,5 @@
video|0|34|1.360000|34|1.360000|1|0.040000|N/A|N/A|150000|1924096|K_|1
 Strings Metadata
 audio|1|65280|1.360000|65280|1.360000|1920|0.040000|N/A|N/A|7680|2074624|K_|1
 Strings Metadata
-0|mpeg2video|0|video|1/25|[0][0][0][0]|0x0000|720|608|0|0|0|0|1:1|45:38|yuv422p|5|tv|unknown|unknown|unknown|topleft|tt|N/A|1|N/A|25/1|25/1|1/25|0|0.000000|N/A|N/A|30000000|N/A|N/A|N/A|N/A|35|0|0|0|0|0|0|0|0|0|0|0|0|0x060A2B340101010501010D001300000000000000000000000000000000000001
+0|mpeg2video|0|video|1/25|[0][0][0][0]|0x0000|720|608|0|0|0|0|1:1|45:38|yuv422p|5|tv|unknown|unknown|unknown|topleft|tb|N/A|1|N/A|25/1|25/1|1/25|0|0.000000|N/A|N/A|30000000|N/A|N/A|N/A|N/A|35|0|0|0|0|0|0|0|0|0|0|0|0|0x060A2B340101010501010D001300000000000000000000000000000000000001
 1|pcm_s16le|unknown|audio|1/48000|[0][0][0][0]|0x0000|s16|48000|2|unknown|16|N/A|0/0|0/0|1/48000|0|0.000000|N/A|N/A|1536000|N/A|N/A|N/A|N/A|35|0|0|0|0|0|0|0|0|0|0|0|0|0x060A2B340101010501010D001300000000000000000000000000000000000001


Patch to test on top of the late_encoder_init set follows (also
included as part of the WIP branch
https://github.com/jeeb/ffmpeg/commits/late_encoder_init_v5 ):

Jan

------->8------
diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 8874da9268..0a3b998e7a 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -1114,7 +1114,6 @@ static void do_video_out(OutputFile *of,
     int ret, format_video_sync;
     AVPacket pkt;
     AVCodecContext *enc = ost->enc_ctx;
-    AVCodecParameters *mux_par = ost->st->codecpar;
     AVRational frame_rate;
     int nb_frames, nb0_frames, i;
     double delta, delta0;
@@ -1276,18 +1275,6 @@ static void do_video_out(OutputFile *of,
         if (!check_recording_time(ost))
             return;

-        if (enc->flags & (AV_CODEC_FLAG_INTERLACED_DCT |
AV_CODEC_FLAG_INTERLACED_ME) &&
-            ost->top_field_first >= 0)
-            in_picture->top_field_first = !!ost->top_field_first;
-
-        if (in_picture->interlaced_frame) {
-            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;
-        } else
-            mux_par->field_order = AV_FIELD_PROGRESSIVE;
-
         in_picture->quality = enc->global_quality;
         in_picture->pict_type = 0;

@@ -3432,6 +3419,20 @@ static int
init_output_stream_encode(OutputStream *ost, AVFrame *frame)
             enc_ctx->field_order = AV_FIELD_TT;
         }

+        if (frame) {
+            if (enc_ctx->flags & (AV_CODEC_FLAG_INTERLACED_DCT |
AV_CODEC_FLAG_INTERLACED_ME) &&
+                ost->top_field_first >= 0)
+                frame->top_field_first = !!ost->top_field_first;
+
+            if (frame->interlaced_frame) {
+                if (enc_ctx->codec->id == AV_CODEC_ID_MJPEG)
+                    enc_ctx->field_order = frame->top_field_first ?
AV_FIELD_TT:AV_FIELD_BB;
+                else
+                    enc_ctx->field_order = frame->top_field_first ?
AV_FIELD_TB:AV_FIELD_BT;
+            } else
+                enc_ctx->field_order = AV_FIELD_PROGRESSIVE;
+        }
+
         if (ost->forced_keyframes) {
             if (!strncmp(ost->forced_keyframes, "expr:", 5)) {
                 ret = av_expr_parse(&ost->forced_keyframes_pexpr,
ost->forced_keyframes+5,
-- 
2.26.2