| Message ID | 7D9B0A5171224A4A9A6308992BC4469B3BC46423@shsmsx102.ccr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
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
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).
> > 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
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 [...]
> 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/ )
> 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".
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