diff mbox series

[FFmpeg-devel,v7,3/3] avcodec/mpeg12dec: Add CPB coded side data

Message ID 20200114234213.3224-4-nicolas.gaullier@cji.paris
State New
Headers show
Series Fix mpeg1/2 stream copy
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Nicolas Gaullier Jan. 14, 2020, 11:42 p.m. UTC
This fixes mpeg2video stream copies to mpeg muxer like this:
  ffmpeg -i xdcamhd.mxf -c:v copy output.mpg
---
 libavcodec/mpeg12dec.c       | 7 +++++++
 tests/ref/fate/mxf-probe-d10 | 3 +++
 tests/ref/fate/ts-demux      | 2 +-
 3 files changed, 11 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer Feb. 7, 2020, 10:38 p.m. UTC | #1
On Wed, Jan 15, 2020 at 12:42:13AM +0100, Nicolas Gaullier wrote:
> This fixes mpeg2video stream copies to mpeg muxer like this:
>   ffmpeg -i xdcamhd.mxf -c:v copy output.mpg
> ---
>  libavcodec/mpeg12dec.c       | 7 +++++++
>  tests/ref/fate/mxf-probe-d10 | 3 +++
>  tests/ref/fate/ts-demux      | 2 +-
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index 17f9495a1d..48ac14fafa 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -1398,6 +1398,7 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
>      MpegEncContext *s = &s1->mpeg_enc_ctx;
>      int horiz_size_ext, vert_size_ext;
>      int bit_rate_ext;
> +    AVCPBProperties *cpb_props;
>  
>      skip_bits(&s->gb, 1); /* profile and level esc*/
>      s->avctx->profile       = get_bits(&s->gb, 3);
> @@ -1429,6 +1430,12 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
>      ff_dlog(s->avctx, "sequence extension\n");
>      s->codec_id = s->avctx->codec_id = AV_CODEC_ID_MPEG2VIDEO;
>  
> +    if (cpb_props = ff_add_cpb_side_data(s->avctx)) {
> +        cpb_props->buffer_size = FFMAX(cpb_props->buffer_size, s->avctx->rc_buffer_size);
> +        if (s->bit_rate != 0x3FFFF*400)
> +            cpb_props->max_bitrate = FFMAX(cpb_props->max_bitrate, s->bit_rate);
> +    }

why does this not export exactly the numbers as read from the header?

thx

[...]
James Almer Feb. 7, 2020, 10:43 p.m. UTC | #2
On 1/14/2020 8:42 PM, Nicolas Gaullier wrote:
> This fixes mpeg2video stream copies to mpeg muxer like this:
>   ffmpeg -i xdcamhd.mxf -c:v copy output.mpg
> ---
>  libavcodec/mpeg12dec.c       | 7 +++++++
>  tests/ref/fate/mxf-probe-d10 | 3 +++
>  tests/ref/fate/ts-demux      | 2 +-
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index 17f9495a1d..48ac14fafa 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -1398,6 +1398,7 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
>      MpegEncContext *s = &s1->mpeg_enc_ctx;
>      int horiz_size_ext, vert_size_ext;
>      int bit_rate_ext;
> +    AVCPBProperties *cpb_props;
>  
>      skip_bits(&s->gb, 1); /* profile and level esc*/
>      s->avctx->profile       = get_bits(&s->gb, 3);
> @@ -1429,6 +1430,12 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
>      ff_dlog(s->avctx, "sequence extension\n");
>      s->codec_id = s->avctx->codec_id = AV_CODEC_ID_MPEG2VIDEO;
>  
> +    if (cpb_props = ff_add_cpb_side_data(s->avctx)) {
> +        cpb_props->buffer_size = FFMAX(cpb_props->buffer_size, s->avctx->rc_buffer_size);

cpb_props->buffer_size is initialized as zero, and rc_buffer_size is not
meant to be used for decoding, so i imagine is also zero.

> +        if (s->bit_rate != 0x3FFFF*400)
> +            cpb_props->max_bitrate = FFMAX(cpb_props->max_bitrate, s->bit_rate);
> +    }
> +
>      if (s->avctx->debug & FF_DEBUG_PICT_INFO)
>          av_log(s->avctx, AV_LOG_DEBUG,
>                 "profile: %d, level: %d ps: %d cf:%d vbv buffer: %d, bitrate:%"PRId64"\n",
> diff --git a/tests/ref/fate/mxf-probe-d10 b/tests/ref/fate/mxf-probe-d10
> index ab564467b5..317d4ae4c5 100644
> --- a/tests/ref/fate/mxf-probe-d10
> +++ b/tests/ref/fate/mxf-probe-d10
> @@ -50,6 +50,9 @@ DISPOSITION:clean_effects=0
>  DISPOSITION:attached_pic=0
>  DISPOSITION:timed_thumbnails=0
>  TAG:file_package_umid=0x060A2B340101010501010D1313000000AE86B200913105800000080046A54011
> +[SIDE_DATA]
> +side_data_type=CPB properties
> +[/SIDE_DATA]
>  [/STREAM]
>  [STREAM]
>  index=1
> diff --git a/tests/ref/fate/ts-demux b/tests/ref/fate/ts-demux
> index eb13ecc684..cdf34d6af0 100644
> --- a/tests/ref/fate/ts-demux
> +++ b/tests/ref/fate/ts-demux
> @@ -15,7 +15,7 @@
>  1,       5760,       5760,     2880,     1536, 0xbab5129c
>  1,       8640,       8640,     2880,     1536, 0x602f034b, S=1,        1, 0x00bd00bd
>  1,      11520,      11520,     2880,      906, 0x69cdcbcd
> -0,      32037,      36541,     1501,   114336, 0x37a215a8, S=1,        1, 0x00e000e0
> +0,      32037,      36541,     1501,   114336, 0x37a215a8, S=2,        1, 0x00e000e0,       24, 0x663d0b52
>  0,      33538,      33538,     1501,    12560, 0xb559a3d4, F=0x0, S=1,        1, 0x00e000e0
>  0,      35040,      35040,     1501,    12704, 0x2614adf4, F=0x0, S=1,        1, 0x00e000e0
>  0,      36541,      41046,     1501,    51976, 0x9ff1dbfe, F=0x0, S=1,        1, 0x00e000e0
>
James Almer Feb. 7, 2020, 10:48 p.m. UTC | #3
On 2/7/2020 7:43 PM, James Almer wrote:
> On 1/14/2020 8:42 PM, Nicolas Gaullier wrote:
>> This fixes mpeg2video stream copies to mpeg muxer like this:
>>   ffmpeg -i xdcamhd.mxf -c:v copy output.mpg
>> ---
>>  libavcodec/mpeg12dec.c       | 7 +++++++
>>  tests/ref/fate/mxf-probe-d10 | 3 +++
>>  tests/ref/fate/ts-demux      | 2 +-
>>  3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
>> index 17f9495a1d..48ac14fafa 100644
>> --- a/libavcodec/mpeg12dec.c
>> +++ b/libavcodec/mpeg12dec.c
>> @@ -1398,6 +1398,7 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
>>      MpegEncContext *s = &s1->mpeg_enc_ctx;
>>      int horiz_size_ext, vert_size_ext;
>>      int bit_rate_ext;
>> +    AVCPBProperties *cpb_props;
>>  
>>      skip_bits(&s->gb, 1); /* profile and level esc*/
>>      s->avctx->profile       = get_bits(&s->gb, 3);
>> @@ -1429,6 +1430,12 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
>>      ff_dlog(s->avctx, "sequence extension\n");
>>      s->codec_id = s->avctx->codec_id = AV_CODEC_ID_MPEG2VIDEO;
>>  
>> +    if (cpb_props = ff_add_cpb_side_data(s->avctx)) {
>> +        cpb_props->buffer_size = FFMAX(cpb_props->buffer_size, s->avctx->rc_buffer_size);
> 
> cpb_props->buffer_size is initialized as zero, and rc_buffer_size is not
> meant to be used for decoding, so i imagine is also zero.

Ok, so i see this decoder is setting rc_buffer_size despite the doxy
stating it shouldn't, so nevermind that part.

I think this should be done using an internal field in Mpeg1Context
instead, or the API changed and it reflected in the doxy. I'm more
inclined for the former option, since you'll be exporting the value
using AVCPBProperties after all.

> 
>> +        if (s->bit_rate != 0x3FFFF*400)
>> +            cpb_props->max_bitrate = FFMAX(cpb_props->max_bitrate, s->bit_rate);
>> +    }
>> +
>>      if (s->avctx->debug & FF_DEBUG_PICT_INFO)
>>          av_log(s->avctx, AV_LOG_DEBUG,
>>                 "profile: %d, level: %d ps: %d cf:%d vbv buffer: %d, bitrate:%"PRId64"\n",
>> diff --git a/tests/ref/fate/mxf-probe-d10 b/tests/ref/fate/mxf-probe-d10
>> index ab564467b5..317d4ae4c5 100644
>> --- a/tests/ref/fate/mxf-probe-d10
>> +++ b/tests/ref/fate/mxf-probe-d10
>> @@ -50,6 +50,9 @@ DISPOSITION:clean_effects=0
>>  DISPOSITION:attached_pic=0
>>  DISPOSITION:timed_thumbnails=0
>>  TAG:file_package_umid=0x060A2B340101010501010D1313000000AE86B200913105800000080046A54011
>> +[SIDE_DATA]
>> +side_data_type=CPB properties
>> +[/SIDE_DATA]
>>  [/STREAM]
>>  [STREAM]
>>  index=1
>> diff --git a/tests/ref/fate/ts-demux b/tests/ref/fate/ts-demux
>> index eb13ecc684..cdf34d6af0 100644
>> --- a/tests/ref/fate/ts-demux
>> +++ b/tests/ref/fate/ts-demux
>> @@ -15,7 +15,7 @@
>>  1,       5760,       5760,     2880,     1536, 0xbab5129c
>>  1,       8640,       8640,     2880,     1536, 0x602f034b, S=1,        1, 0x00bd00bd
>>  1,      11520,      11520,     2880,      906, 0x69cdcbcd
>> -0,      32037,      36541,     1501,   114336, 0x37a215a8, S=1,        1, 0x00e000e0
>> +0,      32037,      36541,     1501,   114336, 0x37a215a8, S=2,        1, 0x00e000e0,       24, 0x663d0b52
>>  0,      33538,      33538,     1501,    12560, 0xb559a3d4, F=0x0, S=1,        1, 0x00e000e0
>>  0,      35040,      35040,     1501,    12704, 0x2614adf4, F=0x0, S=1,        1, 0x00e000e0
>>  0,      36541,      41046,     1501,    51976, 0x9ff1dbfe, F=0x0, S=1,        1, 0x00e000e0
>>
>
Nicolas Gaullier Feb. 20, 2020, 11:22 a.m. UTC | #4
> De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Michael Niedermayer
> Envoyé : vendredi 7 février 2020 23:39
> À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Objet : Re: [FFmpeg-devel] [PATCH v7 3/3] avcodec/mpeg12dec: Add CPB coded side data
> 
> On Wed, Jan 15, 2020 at 12:42:13AM +0100, Nicolas Gaullier wrote:
> > This fixes mpeg2video stream copies to mpeg muxer like this:
> >   ffmpeg -i xdcamhd.mxf -c:v copy output.mpg
> > ---
> >  libavcodec/mpeg12dec.c       | 7 +++++++
> >  tests/ref/fate/mxf-probe-d10 | 3 +++
> >  tests/ref/fate/ts-demux      | 2 +-
> >  3 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index
> > 17f9495a1d..48ac14fafa 100644
> > --- a/libavcodec/mpeg12dec.c
> > +++ b/libavcodec/mpeg12dec.c
> > @@ -1398,6 +1398,7 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
> >      MpegEncContext *s = &s1->mpeg_enc_ctx;
> >      int horiz_size_ext, vert_size_ext;
> >      int bit_rate_ext;
> > +    AVCPBProperties *cpb_props;
> >
> >      skip_bits(&s->gb, 1); /* profile and level esc*/
> >      s->avctx->profile       = get_bits(&s->gb, 3);
> > @@ -1429,6 +1430,12 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
> >      ff_dlog(s->avctx, "sequence extension\n");
> >      s->codec_id = s->avctx->codec_id = AV_CODEC_ID_MPEG2VIDEO;
> >
> > +    if (cpb_props = ff_add_cpb_side_data(s->avctx)) {
> > +        cpb_props->buffer_size = FFMAX(cpb_props->buffer_size, s->avctx->rc_buffer_size);
> > +        if (s->bit_rate != 0x3FFFF*400)
> > +            cpb_props->max_bitrate = FFMAX(cpb_props->max_bitrate, s->bit_rate);
> > +    }
> 
> why does this not export exactly the numbers as read from the header?
> 
> thx
The header values are expressed in units of 400bit/s, and the native value 0x3FFFF is reserved, in case of MPEG-1 (but the code is shared), for vbr signalling.
This is not very nice to read, but this is how it is implemented in current code.
Nicolas
Nicolas Gaullier Feb. 20, 2020, 11:36 a.m. UTC | #5
> De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de James Almer
> Envoyé : vendredi 7 février 2020 23:48
> À : ffmpeg-devel@ffmpeg.org
> Objet : Re: [FFmpeg-devel] [PATCH v7 3/3] avcodec/mpeg12dec: Add CPB coded side data
> 
> On 2/7/2020 7:43 PM, James Almer wrote:
> > On 1/14/2020 8:42 PM, Nicolas Gaullier wrote:
> >> This fixes mpeg2video stream copies to mpeg muxer like this:
> >>   ffmpeg -i xdcamhd.mxf -c:v copy output.mpg
> >> ---
> >>  libavcodec/mpeg12dec.c       | 7 +++++++
> >>  tests/ref/fate/mxf-probe-d10 | 3 +++
> >>  tests/ref/fate/ts-demux      | 2 +-
> >>  3 files changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> >> index 17f9495a1d..48ac14fafa 100644
> >> --- a/libavcodec/mpeg12dec.c
> >> +++ b/libavcodec/mpeg12dec.c
> >> @@ -1398,6 +1398,7 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
> >>      MpegEncContext *s = &s1->mpeg_enc_ctx;
> >>      int horiz_size_ext, vert_size_ext;
> >>      int bit_rate_ext;
> >> +    AVCPBProperties *cpb_props;
> >>
> >>      skip_bits(&s->gb, 1); /* profile and level esc*/
> >>      s->avctx->profile       = get_bits(&s->gb, 3);
> >> @@ -1429,6 +1430,12 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
> >>      ff_dlog(s->avctx, "sequence extension\n");
> >>      s->codec_id = s->avctx->codec_id = AV_CODEC_ID_MPEG2VIDEO;
> >>
> >> +    if (cpb_props = ff_add_cpb_side_data(s->avctx)) {
> >> +        cpb_props->buffer_size = FFMAX(cpb_props->buffer_size, s->avctx->rc_buffer_size);
> >
> > cpb_props->buffer_size is initialized as zero, and rc_buffer_size is not
> > meant to be used for decoding, so i imagine is also zero.
> 
> Ok, so i see this decoder is setting rc_buffer_size despite the doxy
> stating it shouldn't, so nevermind that part.
> 
> I think this should be done using an internal field in Mpeg1Context
> instead, or the API changed and it reflected in the doxy. I'm more
> inclined for the former option, since you'll be exporting the value
> using AVCPBProperties after all.
I agree with you, but this is not related to my patchset, so this should be fixed in another patch.
And I would like not to postpone this again... do you mind if this patch is applied first and if I fix this mpeg12dec design issue later (I am committing myself to do it) ?
Nicolas
Michael Niedermayer Feb. 21, 2020, 11:53 a.m. UTC | #6
On Thu, Feb 20, 2020 at 11:22:33AM +0000, Gaullier Nicolas wrote:
> > De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Michael Niedermayer
> > Envoyé : vendredi 7 février 2020 23:39
> > À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Objet : Re: [FFmpeg-devel] [PATCH v7 3/3] avcodec/mpeg12dec: Add CPB coded side data
> > 
> > On Wed, Jan 15, 2020 at 12:42:13AM +0100, Nicolas Gaullier wrote:
> > > This fixes mpeg2video stream copies to mpeg muxer like this:
> > >   ffmpeg -i xdcamhd.mxf -c:v copy output.mpg
> > > ---
> > >  libavcodec/mpeg12dec.c       | 7 +++++++
> > >  tests/ref/fate/mxf-probe-d10 | 3 +++
> > >  tests/ref/fate/ts-demux      | 2 +-
> > >  3 files changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index
> > > 17f9495a1d..48ac14fafa 100644
> > > --- a/libavcodec/mpeg12dec.c
> > > +++ b/libavcodec/mpeg12dec.c
> > > @@ -1398,6 +1398,7 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
> > >      MpegEncContext *s = &s1->mpeg_enc_ctx;
> > >      int horiz_size_ext, vert_size_ext;
> > >      int bit_rate_ext;
> > > +    AVCPBProperties *cpb_props;
> > >
> > >      skip_bits(&s->gb, 1); /* profile and level esc*/
> > >      s->avctx->profile       = get_bits(&s->gb, 3);
> > > @@ -1429,6 +1430,12 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
> > >      ff_dlog(s->avctx, "sequence extension\n");
> > >      s->codec_id = s->avctx->codec_id = AV_CODEC_ID_MPEG2VIDEO;
> > >
> > > +    if (cpb_props = ff_add_cpb_side_data(s->avctx)) {
> > > +        cpb_props->buffer_size = FFMAX(cpb_props->buffer_size, s->avctx->rc_buffer_size);
> > > +        if (s->bit_rate != 0x3FFFF*400)
> > > +            cpb_props->max_bitrate = FFMAX(cpb_props->max_bitrate, s->bit_rate);
> > > +    }
> > 
> > why does this not export exactly the numbers as read from the header?
> > 
> > thx
> The header values are expressed in units of 400bit/s, and the native value 0x3FFFF is reserved, in case of MPEG-1 (but the code is shared), for vbr signalling.
> This is not very nice to read, but this is how it is implemented in current code.

you misunderstand, why do you take the maximum of several things instead of
exporting the value from the header ?

Thanks

[...]
Nicolas Gaullier Feb. 21, 2020, 1:20 p.m. UTC | #7
> De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Michael Niedermayer
> Envoyé : vendredi 21 février 2020 12:54
> À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Objet : Re: [FFmpeg-devel] [PATCH v7 3/3] avcodec/mpeg12dec: Add CPB coded side data
> 
> On Thu, Feb 20, 2020 at 11:22:33AM +0000, Gaullier Nicolas wrote:
> > > De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Michael Niedermayer
> > > Envoyé : vendredi 7 février 2020 23:39
> > > À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > > Objet : Re: [FFmpeg-devel] [PATCH v7 3/3] avcodec/mpeg12dec: Add CPB coded side data
> > >
> > > On Wed, Jan 15, 2020 at 12:42:13AM +0100, Nicolas Gaullier wrote:
> > > > This fixes mpeg2video stream copies to mpeg muxer like this:
> > > >   ffmpeg -i xdcamhd.mxf -c:v copy output.mpg
> > > > ---
> > > >  libavcodec/mpeg12dec.c       | 7 +++++++
> > > >  tests/ref/fate/mxf-probe-d10 | 3 +++
> > > >  tests/ref/fate/ts-demux      | 2 +-
> > > >  3 files changed, 11 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index
> > > > 17f9495a1d..48ac14fafa 100644
> > > > --- a/libavcodec/mpeg12dec.c
> > > > +++ b/libavcodec/mpeg12dec.c
> > > > @@ -1398,6 +1398,7 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
> > > >      MpegEncContext *s = &s1->mpeg_enc_ctx;
> > > >      int horiz_size_ext, vert_size_ext;
> > > >      int bit_rate_ext;
> > > > +    AVCPBProperties *cpb_props;
> > > >
> > > >      skip_bits(&s->gb, 1); /* profile and level esc*/
> > > >      s->avctx->profile       = get_bits(&s->gb, 3);
> > > > @@ -1429,6 +1430,12 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
> > > >      ff_dlog(s->avctx, "sequence extension\n");
> > > >      s->codec_id = s->avctx->codec_id = AV_CODEC_ID_MPEG2VIDEO;
> > > >
> > > > +    if (cpb_props = ff_add_cpb_side_data(s->avctx)) {
> > > > +        cpb_props->buffer_size = FFMAX(cpb_props->buffer_size, s->avctx->rc_buffer_size);
> > > > +        if (s->bit_rate != 0x3FFFF*400)
> > > > +            cpb_props->max_bitrate = FFMAX(cpb_props->max_bitrate, s->bit_rate);
> > > > +    }
> > >
> > > why does this not export exactly the numbers as read from the header?
> > >
> > > thx
> > The header values are expressed in units of 400bit/s, and the native value 0x3FFFF is reserved, in case of MPEG-1 (but the code is
> shared), for vbr signalling.
> > This is not very nice to read, but this is how it is implemented in current code.
> 
> you misunderstand, why do you take the maximum of several things instead of
> exporting the value from the header ?
> 
> Thanks
Sorry for my misunderstanding. I thought the cpb properties had to reflect the entire stream at the end and thus cumulate the size/max values.
I agree it is best to have an exact match with native header values.
Thank you for your feedback.
I will send a new version with the 2x FFMAX removed. 
Nicolas
Michael Niedermayer Feb. 22, 2020, 5:02 p.m. UTC | #8
On Fri, Feb 21, 2020 at 01:20:47PM +0000, Gaullier Nicolas wrote:
> > De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Michael Niedermayer
> > Envoyé : vendredi 21 février 2020 12:54
> > À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Objet : Re: [FFmpeg-devel] [PATCH v7 3/3] avcodec/mpeg12dec: Add CPB coded side data
> > 
> > On Thu, Feb 20, 2020 at 11:22:33AM +0000, Gaullier Nicolas wrote:
> > > > De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Michael Niedermayer
> > > > Envoyé : vendredi 7 février 2020 23:39
> > > > À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > > > Objet : Re: [FFmpeg-devel] [PATCH v7 3/3] avcodec/mpeg12dec: Add CPB coded side data
> > > >
> > > > On Wed, Jan 15, 2020 at 12:42:13AM +0100, Nicolas Gaullier wrote:
> > > > > This fixes mpeg2video stream copies to mpeg muxer like this:
> > > > >   ffmpeg -i xdcamhd.mxf -c:v copy output.mpg
> > > > > ---
> > > > >  libavcodec/mpeg12dec.c       | 7 +++++++
> > > > >  tests/ref/fate/mxf-probe-d10 | 3 +++
> > > > >  tests/ref/fate/ts-demux      | 2 +-
> > > > >  3 files changed, 11 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index
> > > > > 17f9495a1d..48ac14fafa 100644
> > > > > --- a/libavcodec/mpeg12dec.c
> > > > > +++ b/libavcodec/mpeg12dec.c
> > > > > @@ -1398,6 +1398,7 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
> > > > >      MpegEncContext *s = &s1->mpeg_enc_ctx;
> > > > >      int horiz_size_ext, vert_size_ext;
> > > > >      int bit_rate_ext;
> > > > > +    AVCPBProperties *cpb_props;
> > > > >
> > > > >      skip_bits(&s->gb, 1); /* profile and level esc*/
> > > > >      s->avctx->profile       = get_bits(&s->gb, 3);
> > > > > @@ -1429,6 +1430,12 @@ static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
> > > > >      ff_dlog(s->avctx, "sequence extension\n");
> > > > >      s->codec_id = s->avctx->codec_id = AV_CODEC_ID_MPEG2VIDEO;
> > > > >
> > > > > +    if (cpb_props = ff_add_cpb_side_data(s->avctx)) {
> > > > > +        cpb_props->buffer_size = FFMAX(cpb_props->buffer_size, s->avctx->rc_buffer_size);
> > > > > +        if (s->bit_rate != 0x3FFFF*400)
> > > > > +            cpb_props->max_bitrate = FFMAX(cpb_props->max_bitrate, s->bit_rate);
> > > > > +    }
> > > >
> > > > why does this not export exactly the numbers as read from the header?
> > > >
> > > > thx
> > > The header values are expressed in units of 400bit/s, and the native value 0x3FFFF is reserved, in case of MPEG-1 (but the code is
> > shared), for vbr signalling.
> > > This is not very nice to read, but this is how it is implemented in current code.
> > 
> > you misunderstand, why do you take the maximum of several things instead of
> > exporting the value from the header ?
> > 
> > Thanks
> Sorry for my misunderstanding. I thought the cpb properties had to reflect the entire stream at the end and thus cumulate the size/max values.

well, i see your point but that would not work in practice because in a
scenario where a stream is remuxed the output properties would be written in
regular intervalls before the input is read to the end so that would not
work.

Outputing the values as is would at least preserve the values as they are


> I agree it is best to have an exact match with native header values.
> Thank you for your feedback.
> I will send a new version with the 2x FFMAX removed. 
> Nicolas

[...]
diff mbox series

Patch

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 17f9495a1d..48ac14fafa 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1398,6 +1398,7 @@  static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
     MpegEncContext *s = &s1->mpeg_enc_ctx;
     int horiz_size_ext, vert_size_ext;
     int bit_rate_ext;
+    AVCPBProperties *cpb_props;
 
     skip_bits(&s->gb, 1); /* profile and level esc*/
     s->avctx->profile       = get_bits(&s->gb, 3);
@@ -1429,6 +1430,12 @@  static void mpeg_decode_sequence_extension(Mpeg1Context *s1)
     ff_dlog(s->avctx, "sequence extension\n");
     s->codec_id = s->avctx->codec_id = AV_CODEC_ID_MPEG2VIDEO;
 
+    if (cpb_props = ff_add_cpb_side_data(s->avctx)) {
+        cpb_props->buffer_size = FFMAX(cpb_props->buffer_size, s->avctx->rc_buffer_size);
+        if (s->bit_rate != 0x3FFFF*400)
+            cpb_props->max_bitrate = FFMAX(cpb_props->max_bitrate, s->bit_rate);
+    }
+
     if (s->avctx->debug & FF_DEBUG_PICT_INFO)
         av_log(s->avctx, AV_LOG_DEBUG,
                "profile: %d, level: %d ps: %d cf:%d vbv buffer: %d, bitrate:%"PRId64"\n",
diff --git a/tests/ref/fate/mxf-probe-d10 b/tests/ref/fate/mxf-probe-d10
index ab564467b5..317d4ae4c5 100644
--- a/tests/ref/fate/mxf-probe-d10
+++ b/tests/ref/fate/mxf-probe-d10
@@ -50,6 +50,9 @@  DISPOSITION:clean_effects=0
 DISPOSITION:attached_pic=0
 DISPOSITION:timed_thumbnails=0
 TAG:file_package_umid=0x060A2B340101010501010D1313000000AE86B200913105800000080046A54011
+[SIDE_DATA]
+side_data_type=CPB properties
+[/SIDE_DATA]
 [/STREAM]
 [STREAM]
 index=1
diff --git a/tests/ref/fate/ts-demux b/tests/ref/fate/ts-demux
index eb13ecc684..cdf34d6af0 100644
--- a/tests/ref/fate/ts-demux
+++ b/tests/ref/fate/ts-demux
@@ -15,7 +15,7 @@ 
 1,       5760,       5760,     2880,     1536, 0xbab5129c
 1,       8640,       8640,     2880,     1536, 0x602f034b, S=1,        1, 0x00bd00bd
 1,      11520,      11520,     2880,      906, 0x69cdcbcd
-0,      32037,      36541,     1501,   114336, 0x37a215a8, S=1,        1, 0x00e000e0
+0,      32037,      36541,     1501,   114336, 0x37a215a8, S=2,        1, 0x00e000e0,       24, 0x663d0b52
 0,      33538,      33538,     1501,    12560, 0xb559a3d4, F=0x0, S=1,        1, 0x00e000e0
 0,      35040,      35040,     1501,    12704, 0x2614adf4, F=0x0, S=1,        1, 0x00e000e0
 0,      36541,      41046,     1501,    51976, 0x9ff1dbfe, F=0x0, S=1,        1, 0x00e000e0