[FFmpeg-devel,1/2] lavc: Add coded_w/h to AVCodecParameters

Submitted by Zhong Li on Jan. 19, 2018, 3:19 a.m.

Details

Message ID 7D9B0A5171224A4A9A6308992BC4469B3BC46423@shsmsx102.ccr.corp.intel.com
State New
Headers show

Commit Message

Zhong Li Jan. 19, 2018, 3:19 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of James Almer

> Sent: Thursday, January 18, 2018 1:15 PM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc: Add coded_w/h to

> AVCodecParameters

> 

> On 1/18/2018 2:03 AM, Zhong Li wrote:

> > coded_width/height may be different from width/height sometimes

> 

> > (e.g, crop or lowres cases).

> 

> Which is why it's not a field that belongs to AVCodecParameters.

> 

> Codec level cropping has nothing to do with containers. Same with lowres,

> which is an internal feature, and scheduled for removal.


Got it. How about fixing ticket #6958 as below? 

ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Comments

Hendrik Leppkes Jan. 19, 2018, 10:12 a.m.
On Fri, Jan 19, 2018 at 4:19 AM, Li, Zhong <zhong.li@intel.com> wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
>> Of James Almer
>> Sent: Thursday, January 18, 2018 1:15 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc: Add coded_w/h to
>> AVCodecParameters
>>
>> On 1/18/2018 2:03 AM, Zhong Li wrote:
>> > coded_width/height may be different from width/height sometimes
>>
>> > (e.g, crop or lowres cases).
>>
>> Which is why it's not a field that belongs to AVCodecParameters.
>>
>> Codec level cropping has nothing to do with containers. Same with lowres,
>> which is an internal feature, and scheduled for removal.
>
> Got it. How about fixing ticket #6958 as below?
>
> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> index 0e7a771..233760d 100644
> --- a/fftools/ffprobe.c
> +++ b/fftools/ffprobe.c
> @@ -2512,10 +2512,12 @@ static int show_stream(WriterContext *w, AVFormatContext *fmt_ctx, int stream_id
>      case AVMEDIA_TYPE_VIDEO:
>          print_int("width",        par->width);
>          print_int("height",       par->height);
> +#if FF_API_LAVF_AVCTX
>          if (dec_ctx) {
>              print_int("coded_width",  dec_ctx->coded_width);
>              print_int("coded_height", dec_ctx->coded_height);
>          }
> +#endif
>          print_int("has_b_frames", par->video_delay);
>          sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
>          if (sar.den) {
> @@ -2912,6 +2914,10 @@ static int open_input_file(InputFile *ifile, const char *filename)
>
>              ist->dec_ctx->pkt_timebase = stream->time_base;
>              ist->dec_ctx->framerate = stream->avg_frame_rate;
> +#if FF_API_LAVF_AVCTX
> +            ist->dec_ctx->coded_width = stream->codec->coded_width;
> +            ist->dec_ctx->coded_height = stream->codec->coded_height;
> +#endif
>

As mentioned in the thread for that patch already, writing new code
using deprecated API should really be avoided.

The way I see it, if someone really needs to know coded w/h (which is
typically an internal technical detail of no relevance to users), they
should decode a frame and get it from the decoder.

- Hendrik
James Almer Jan. 19, 2018, 4:15 p.m.
On 1/19/2018 7:12 AM, Hendrik Leppkes wrote:
> On Fri, Jan 19, 2018 at 4:19 AM, Li, Zhong <zhong.li@intel.com> wrote:
>>> -----Original Message-----
>>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
>>> Of James Almer
>>> Sent: Thursday, January 18, 2018 1:15 PM
>>> To: ffmpeg-devel@ffmpeg.org
>>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc: Add coded_w/h to
>>> AVCodecParameters
>>>
>>> On 1/18/2018 2:03 AM, Zhong Li wrote:
>>>> coded_width/height may be different from width/height sometimes
>>>
>>>> (e.g, crop or lowres cases).
>>>
>>> Which is why it's not a field that belongs to AVCodecParameters.
>>>
>>> Codec level cropping has nothing to do with containers. Same with lowres,
>>> which is an internal feature, and scheduled for removal.
>>
>> Got it. How about fixing ticket #6958 as below?
>>
>> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
>> index 0e7a771..233760d 100644
>> --- a/fftools/ffprobe.c
>> +++ b/fftools/ffprobe.c
>> @@ -2512,10 +2512,12 @@ static int show_stream(WriterContext *w, AVFormatContext *fmt_ctx, int stream_id
>>      case AVMEDIA_TYPE_VIDEO:
>>          print_int("width",        par->width);
>>          print_int("height",       par->height);
>> +#if FF_API_LAVF_AVCTX
>>          if (dec_ctx) {
>>              print_int("coded_width",  dec_ctx->coded_width);
>>              print_int("coded_height", dec_ctx->coded_height);
>>          }
>> +#endif
>>          print_int("has_b_frames", par->video_delay);
>>          sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
>>          if (sar.den) {
>> @@ -2912,6 +2914,10 @@ static int open_input_file(InputFile *ifile, const char *filename)
>>
>>              ist->dec_ctx->pkt_timebase = stream->time_base;
>>              ist->dec_ctx->framerate = stream->avg_frame_rate;
>> +#if FF_API_LAVF_AVCTX
>> +            ist->dec_ctx->coded_width = stream->codec->coded_width;
>> +            ist->dec_ctx->coded_height = stream->codec->coded_height;
>> +#endif
>>
> 
> As mentioned in the thread for that patch already, writing new code
> using deprecated API should really be avoided.
> 
> The way I see it, if someone really needs to know coded w/h (which is
> typically an internal technical detail of no relevance to users), they
> should decode a frame and get it from the decoder.
> 
> - Hendrik

This specific approach is IMO acceptable. It basically recovers the
pre-codecpar behavior until AVStream->codec is removed, and effectively
fixes the "regression".
Once that's gone, ffprobe will stop reporting coded_width/height
altogether instead of doing so with a bogus value, as everything is
being wrapped in the proper checks.

The alternative is to remove the two print_int() lines right now, of
course, but i don't think it's the best approach when the above patch is
simple, clean, and local to ffprobe.c (unlike the original suggestion).
Zhong Li Jan. 22, 2018, 2:05 a.m.
> 

> As mentioned in the thread for that patch already, writing new code using

> deprecated API should really be avoided.

> 

> The way I see it, if someone really needs to know coded w/h (which is

> typically an internal technical detail of no relevance to users), they should

> decode a frame and get it from the decoder.


James posted some comments on https://patchwork.ffmpeg.org/patch/7342/ , please let me know if you have any other comment. 
Such code was added to remove printing coded_w/h once the API is removed:
#if FF_API_LAVF_AVCTX
          if (dec_ctx) {
              print_int("coded_width",  dec_ctx->coded_width);
              print_int("coded_height", dec_ctx->coded_height);
          }
+#endif
Michael Niedermayer Jan. 26, 2018, 3:55 p.m.
On Fri, Jan 19, 2018 at 01:15:13PM -0300, James Almer wrote:
> On 1/19/2018 7:12 AM, Hendrik Leppkes wrote:
> > On Fri, Jan 19, 2018 at 4:19 AM, Li, Zhong <zhong.li@intel.com> wrote:
> >>> -----Original Message-----
> >>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
> >>> Of James Almer
> >>> Sent: Thursday, January 18, 2018 1:15 PM
> >>> To: ffmpeg-devel@ffmpeg.org
> >>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc: Add coded_w/h to
> >>> AVCodecParameters
> >>>
> >>> On 1/18/2018 2:03 AM, Zhong Li wrote:
> >>>> coded_width/height may be different from width/height sometimes
> >>>
> >>>> (e.g, crop or lowres cases).
> >>>
> >>> Which is why it's not a field that belongs to AVCodecParameters.
> >>>
> >>> Codec level cropping has nothing to do with containers. Same with lowres,
> >>> which is an internal feature, and scheduled for removal.
> >>
> >> Got it. How about fixing ticket #6958 as below?
> >>
> >> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> >> index 0e7a771..233760d 100644
> >> --- a/fftools/ffprobe.c
> >> +++ b/fftools/ffprobe.c
> >> @@ -2512,10 +2512,12 @@ static int show_stream(WriterContext *w, AVFormatContext *fmt_ctx, int stream_id
> >>      case AVMEDIA_TYPE_VIDEO:
> >>          print_int("width",        par->width);
> >>          print_int("height",       par->height);
> >> +#if FF_API_LAVF_AVCTX
> >>          if (dec_ctx) {
> >>              print_int("coded_width",  dec_ctx->coded_width);
> >>              print_int("coded_height", dec_ctx->coded_height);
> >>          }
> >> +#endif
> >>          print_int("has_b_frames", par->video_delay);
> >>          sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
> >>          if (sar.den) {
> >> @@ -2912,6 +2914,10 @@ static int open_input_file(InputFile *ifile, const char *filename)
> >>
> >>              ist->dec_ctx->pkt_timebase = stream->time_base;
> >>              ist->dec_ctx->framerate = stream->avg_frame_rate;
> >> +#if FF_API_LAVF_AVCTX
> >> +            ist->dec_ctx->coded_width = stream->codec->coded_width;
> >> +            ist->dec_ctx->coded_height = stream->codec->coded_height;
> >> +#endif
> >>
> > 
> > As mentioned in the thread for that patch already, writing new code
> > using deprecated API should really be avoided.
> > 
> > The way I see it, if someone really needs to know coded w/h (which is
> > typically an internal technical detail of no relevance to users), they
> > should decode a frame and get it from the decoder.
> > 
> > - Hendrik
> 
> This specific approach is IMO acceptable. It basically recovers the
> pre-codecpar behavior until AVStream->codec is removed, and effectively
> fixes the "regression".
> Once that's gone, ffprobe will stop reporting coded_width/height
> altogether instead of doing so with a bogus value, as everything is
> being wrapped in the proper checks.

+1

[...]
Zhong Li Jan. 30, 2018, 2:05 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
> Of Michael Niedermayer
> Sent: Friday, January 26, 2018 11:56 PM
> To: FFmpeg development discussions and patches
> <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc: Add coded_w/h to
> AVCodecParameters
> 
> On Fri, Jan 19, 2018 at 01:15:13PM -0300, James Almer wrote:
> > On 1/19/2018 7:12 AM, Hendrik Leppkes wrote:
> > > On Fri, Jan 19, 2018 at 4:19 AM, Li, Zhong <zhong.li@intel.com> wrote:
> > >>> -----Original Message-----
> > >>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On
> > >>> Behalf Of James Almer
> > >>> Sent: Thursday, January 18, 2018 1:15 PM
> > >>> To: ffmpeg-devel@ffmpeg.org
> > >>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc: Add coded_w/h to
> > >>> AVCodecParameters
> > >>>
> > >>> On 1/18/2018 2:03 AM, Zhong Li wrote:
> > >>>> coded_width/height may be different from width/height sometimes
> > >>>
> > >>>> (e.g, crop or lowres cases).
> > >>>
> > >>> Which is why it's not a field that belongs to AVCodecParameters.
> > >>>
> > >>> Codec level cropping has nothing to do with containers. Same with
> > >>> lowres, which is an internal feature, and scheduled for removal.
> > >>
> > >> Got it. How about fixing ticket #6958 as below?
> > >>
> > >> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c index
> > >> 0e7a771..233760d 100644
> > >> --- a/fftools/ffprobe.c
> > >> +++ b/fftools/ffprobe.c
> > >> @@ -2512,10 +2512,12 @@ static int show_stream(WriterContext *w,
> AVFormatContext *fmt_ctx, int stream_id
> > >>      case AVMEDIA_TYPE_VIDEO:
> > >>          print_int("width",        par->width);
> > >>          print_int("height",       par->height);
> > >> +#if FF_API_LAVF_AVCTX
> > >>          if (dec_ctx) {
> > >>              print_int("coded_width",  dec_ctx->coded_width);
> > >>              print_int("coded_height", dec_ctx->coded_height);
> > >>          }
> > >> +#endif
> > >>          print_int("has_b_frames", par->video_delay);
> > >>          sar = av_guess_sample_aspect_ratio(fmt_ctx, stream,
> NULL);
> > >>          if (sar.den) {
> > >> @@ -2912,6 +2914,10 @@ static int open_input_file(InputFile *ifile,
> > >> const char *filename)
> > >>
> > >>              ist->dec_ctx->pkt_timebase = stream->time_base;
> > >>              ist->dec_ctx->framerate = stream->avg_frame_rate;
> > >> +#if FF_API_LAVF_AVCTX
> > >> +            ist->dec_ctx->coded_width =
> stream->codec->coded_width;
> > >> +            ist->dec_ctx->coded_height =
> > >> +stream->codec->coded_height; #endif
> > >>
> > >
> > > As mentioned in the thread for that patch already, writing new code
> > > using deprecated API should really be avoided.
> > >
> > > The way I see it, if someone really needs to know coded w/h (which
> > > is typically an internal technical detail of no relevance to users),
> > > they should decode a frame and get it from the decoder.
> > >
> > > - Hendrik
> >
> > This specific approach is IMO acceptable. It basically recovers the
> > pre-codecpar behavior until AVStream->codec is removed, and
> > effectively fixes the "regression".
> > Once that's gone, ffprobe will stop reporting coded_width/height
> > altogether instead of doing so with a bogus value, as everything is
> > being wrapped in the proper checks.
> 
> +1
> 
> [...]
> --
> Michael     GnuPG fingerprint:
> 9FF2128B147EF6730BADF133611EC787040B0FAB

Thanks for Hendrik/James/ Michael's review. 
Will this patch be applied? (Another thread maybe easier to apply: https://patchwork.ffmpeg.org/patch/7344/ )
Zhong Li Feb. 2, 2018, 3:09 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Li, Zhong

> Sent: Tuesday, January 30, 2018 10:06 AM

> To: FFmpeg development discussions and patches

> <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc: Add coded_w/h to

> AVCodecParameters

> 

> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> > Of Michael Niedermayer

> > Sent: Friday, January 26, 2018 11:56 PM

> > To: FFmpeg development discussions and patches

> > <ffmpeg-devel@ffmpeg.org>

> > Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc: Add coded_w/h to

> > AVCodecParameters

> >

> > On Fri, Jan 19, 2018 at 01:15:13PM -0300, James Almer wrote:

> > > On 1/19/2018 7:12 AM, Hendrik Leppkes wrote:

> > > > On Fri, Jan 19, 2018 at 4:19 AM, Li, Zhong <zhong.li@intel.com> wrote:

> > > >>> -----Original Message-----

> > > >>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> > > >>> Behalf Of James Almer

> > > >>> Sent: Thursday, January 18, 2018 1:15 PM

> > > >>> To: ffmpeg-devel@ffmpeg.org

> > > >>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc: Add coded_w/h to

> > > >>> AVCodecParameters

> > > >>>

> > > >>> On 1/18/2018 2:03 AM, Zhong Li wrote:

> > > >>>> coded_width/height may be different from width/height sometimes

> > > >>>

> > > >>>> (e.g, crop or lowres cases).

> > > >>>

> > > >>> Which is why it's not a field that belongs to AVCodecParameters.

> > > >>>

> > > >>> Codec level cropping has nothing to do with containers. Same

> > > >>> with lowres, which is an internal feature, and scheduled for removal.

> > > >>

> > > >> Got it. How about fixing ticket #6958 as below?

> > > >>

> > > >> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c index

> > > >> 0e7a771..233760d 100644

> > > >> --- a/fftools/ffprobe.c

> > > >> +++ b/fftools/ffprobe.c

> > > >> @@ -2512,10 +2512,12 @@ static int show_stream(WriterContext

> *w,

> > AVFormatContext *fmt_ctx, int stream_id

> > > >>      case AVMEDIA_TYPE_VIDEO:

> > > >>          print_int("width",        par->width);

> > > >>          print_int("height",       par->height);

> > > >> +#if FF_API_LAVF_AVCTX

> > > >>          if (dec_ctx) {

> > > >>              print_int("coded_width",  dec_ctx->coded_width);

> > > >>              print_int("coded_height", dec_ctx->coded_height);

> > > >>          }

> > > >> +#endif

> > > >>          print_int("has_b_frames", par->video_delay);

> > > >>          sar = av_guess_sample_aspect_ratio(fmt_ctx, stream,

> > NULL);

> > > >>          if (sar.den) {

> > > >> @@ -2912,6 +2914,10 @@ static int open_input_file(InputFile

> > > >> *ifile, const char *filename)

> > > >>

> > > >>              ist->dec_ctx->pkt_timebase = stream->time_base;

> > > >>              ist->dec_ctx->framerate = stream->avg_frame_rate;

> > > >> +#if FF_API_LAVF_AVCTX

> > > >> +            ist->dec_ctx->coded_width =

> > stream->codec->coded_width;

> > > >> +            ist->dec_ctx->coded_height =

> > > >> +stream->codec->coded_height; #endif

> > > >>

> > > >

> > > > As mentioned in the thread for that patch already, writing new

> > > > code using deprecated API should really be avoided.

> > > >

> > > > The way I see it, if someone really needs to know coded w/h (which

> > > > is typically an internal technical detail of no relevance to

> > > > users), they should decode a frame and get it from the decoder.

> > > >

> > > > - Hendrik

> > >

> > > This specific approach is IMO acceptable. It basically recovers the

> > > pre-codecpar behavior until AVStream->codec is removed, and

> > > effectively fixes the "regression".

> > > Once that's gone, ffprobe will stop reporting coded_width/height

> > > altogether instead of doing so with a bogus value, as everything is

> > > being wrapped in the proper checks.

> >

> > +1

> >

> > [...]

> > --

> > Michael     GnuPG fingerprint:

> > 9FF2128B147EF6730BADF133611EC787040B0FAB

> 

> Thanks for Hendrik/James/ Michael's review.

> Will this patch be applied? (Another thread maybe easier to apply:

> https://patchwork.ffmpeg.org/patch/7344/ )


Ping.
I see the priority of ticket #6958 has been set as "important".

Patch hide | download patch | download mbox

diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index 0e7a771..233760d 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -2512,10 +2512,12 @@  static int show_stream(WriterContext *w, AVFormatContext *fmt_ctx, int stream_id
     case AVMEDIA_TYPE_VIDEO:
         print_int("width",        par->width);
         print_int("height",       par->height);
+#if FF_API_LAVF_AVCTX
         if (dec_ctx) {
             print_int("coded_width",  dec_ctx->coded_width);
             print_int("coded_height", dec_ctx->coded_height);
         }
+#endif
         print_int("has_b_frames", par->video_delay);
         sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
         if (sar.den) {
@@ -2912,6 +2914,10 @@  static int open_input_file(InputFile *ifile, const char *filename)

             ist->dec_ctx->pkt_timebase = stream->time_base;
             ist->dec_ctx->framerate = stream->avg_frame_rate;
+#if FF_API_LAVF_AVCTX
+            ist->dec_ctx->coded_width = stream->codec->coded_width;
+            ist->dec_ctx->coded_height = stream->codec->coded_height;
+#endif

_______________________________________________
ffmpeg-devel mailing list