| Message ID | 20200903185505.1865-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | f901d75bf18c866933f90c052fb990736aee7c47 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/yuv4mpegenc: simplify writing the header | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 9/3/20, James Almer <jamrial@gmail.com> wrote: > Actually write it in yuv4_write_header() instead of with the first > packet. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/yuv4mpegenc.c | 35 ++++++++++++++--------------------- > 1 file changed, 14 insertions(+), 21 deletions(-) > Nice catch, LGTM > diff --git a/libavformat/yuv4mpegenc.c b/libavformat/yuv4mpegenc.c > index e84dbf9568..c4781042bd 100644 > --- a/libavformat/yuv4mpegenc.c > +++ b/libavformat/yuv4mpegenc.c > @@ -26,14 +26,16 @@ > > #define Y4M_LINE_MAX 256 > > -static int yuv4_generate_header(AVFormatContext *s, char* buf) > +static int yuv4_write_header(AVFormatContext *s) > { > AVStream *st; > + AVIOContext *pb = s->pb; > int width, height; > int raten, rated, aspectn, aspectd, n; > char inter; > const char *colorspace = ""; > const char *colorrange = ""; > + char buf[Y4M_LINE_MAX + 1]; > int field_order; > > st = s->streams[0]; > @@ -173,7 +175,15 @@ static int yuv4_generate_header(AVFormatContext *s, > char* buf) > Y4M_MAGIC, width, height, raten, rated, inter, > aspectn, aspectd, colorspace, colorrange); > > - return n; > + if (n < 0) { > + av_log(s, AV_LOG_ERROR, > + "Error. YUV4MPEG stream header write failed.\n"); > + return AVERROR(EIO); > + } > + > + avio_write(pb, buf, strlen(buf)); > + > + return 0; > } > > > @@ -182,26 +192,12 @@ static int yuv4_write_packet(AVFormatContext *s, > AVPacket *pkt) > AVStream *st = s->streams[pkt->stream_index]; > AVIOContext *pb = s->pb; > AVFrame *frame; > - int* first_pkt = s->priv_data; > int width, height, h_chroma_shift, v_chroma_shift; > int i; > - char buf2[Y4M_LINE_MAX + 1]; > uint8_t *ptr, *ptr1, *ptr2; > > frame = (AVFrame *)pkt->data; > > - /* for the first packet we have to output the header as well */ > - if (*first_pkt) { > - *first_pkt = 0; > - if (yuv4_generate_header(s, buf2) < 0) { > - av_log(s, AV_LOG_ERROR, > - "Error. YUV4MPEG stream header write failed.\n"); > - return AVERROR(EIO); > - } else { > - avio_write(pb, buf2, strlen(buf2)); > - } > - } > - > /* construct frame header */ > > avio_printf(s->pb, "%s\n", Y4M_FRAME_MAGIC); > @@ -279,10 +275,8 @@ static int yuv4_write_packet(AVFormatContext *s, > AVPacket *pkt) > return 0; > } > > -static int yuv4_write_header(AVFormatContext *s) > +static int yuv4_init(AVFormatContext *s) > { > - int *first_pkt = s->priv_data; > - > if (s->nb_streams != 1) > return AVERROR(EIO); > > @@ -347,7 +341,6 @@ static int yuv4_write_header(AVFormatContext *s) > return AVERROR(EIO); > } > > - *first_pkt = 1; > return 0; > } > > @@ -355,9 +348,9 @@ AVOutputFormat ff_yuv4mpegpipe_muxer = { > .name = "yuv4mpegpipe", > .long_name = NULL_IF_CONFIG_SMALL("YUV4MPEG pipe"), > .extensions = "y4m", > - .priv_data_size = sizeof(int), > .audio_codec = AV_CODEC_ID_NONE, > .video_codec = AV_CODEC_ID_WRAPPED_AVFRAME, > + .init = yuv4_init, > .write_header = yuv4_write_header, > .write_packet = yuv4_write_packet, > }; > -- > 2.27.0 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 9/3/2020 4:16 PM, Paul B Mahol wrote: > On 9/3/20, James Almer <jamrial@gmail.com> wrote: >> Actually write it in yuv4_write_header() instead of with the first >> packet. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/yuv4mpegenc.c | 35 ++++++++++++++--------------------- >> 1 file changed, 14 insertions(+), 21 deletions(-) >> > > Nice catch, LGTM Pushed, thanks.
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@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 [...]
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@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.
On Thu, Sep 10, 2020 at 3:00 AM James Almer <jamrial@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@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,
diff --git a/libavformat/yuv4mpegenc.c b/libavformat/yuv4mpegenc.c index e84dbf9568..c4781042bd 100644 --- a/libavformat/yuv4mpegenc.c +++ b/libavformat/yuv4mpegenc.c @@ -26,14 +26,16 @@ #define Y4M_LINE_MAX 256 -static int yuv4_generate_header(AVFormatContext *s, char* buf) +static int yuv4_write_header(AVFormatContext *s) { AVStream *st; + AVIOContext *pb = s->pb; int width, height; int raten, rated, aspectn, aspectd, n; char inter; const char *colorspace = ""; const char *colorrange = ""; + char buf[Y4M_LINE_MAX + 1]; int field_order; st = s->streams[0]; @@ -173,7 +175,15 @@ static int yuv4_generate_header(AVFormatContext *s, char* buf) Y4M_MAGIC, width, height, raten, rated, inter, aspectn, aspectd, colorspace, colorrange); - return n; + if (n < 0) { + av_log(s, AV_LOG_ERROR, + "Error. YUV4MPEG stream header write failed.\n"); + return AVERROR(EIO); + } + + avio_write(pb, buf, strlen(buf)); + + return 0; } @@ -182,26 +192,12 @@ static int yuv4_write_packet(AVFormatContext *s, AVPacket *pkt) AVStream *st = s->streams[pkt->stream_index]; AVIOContext *pb = s->pb; AVFrame *frame; - int* first_pkt = s->priv_data; int width, height, h_chroma_shift, v_chroma_shift; int i; - char buf2[Y4M_LINE_MAX + 1]; uint8_t *ptr, *ptr1, *ptr2; frame = (AVFrame *)pkt->data; - /* for the first packet we have to output the header as well */ - if (*first_pkt) { - *first_pkt = 0; - if (yuv4_generate_header(s, buf2) < 0) { - av_log(s, AV_LOG_ERROR, - "Error. YUV4MPEG stream header write failed.\n"); - return AVERROR(EIO); - } else { - avio_write(pb, buf2, strlen(buf2)); - } - } - /* construct frame header */ avio_printf(s->pb, "%s\n", Y4M_FRAME_MAGIC); @@ -279,10 +275,8 @@ static int yuv4_write_packet(AVFormatContext *s, AVPacket *pkt) return 0; } -static int yuv4_write_header(AVFormatContext *s) +static int yuv4_init(AVFormatContext *s) { - int *first_pkt = s->priv_data; - if (s->nb_streams != 1) return AVERROR(EIO); @@ -347,7 +341,6 @@ static int yuv4_write_header(AVFormatContext *s) return AVERROR(EIO); } - *first_pkt = 1; return 0; } @@ -355,9 +348,9 @@ AVOutputFormat ff_yuv4mpegpipe_muxer = { .name = "yuv4mpegpipe", .long_name = NULL_IF_CONFIG_SMALL("YUV4MPEG pipe"), .extensions = "y4m", - .priv_data_size = sizeof(int), .audio_codec = AV_CODEC_ID_NONE, .video_codec = AV_CODEC_ID_WRAPPED_AVFRAME, + .init = yuv4_init, .write_header = yuv4_write_header, .write_packet = yuv4_write_packet, };
Actually write it in yuv4_write_header() instead of with the first packet. Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/yuv4mpegenc.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-)