[FFmpeg-devel] Don't overwrite previously setup dimensions for all codecs

Submitted by Zhong Li on Jan. 10, 2018, 8:10 a.m.

Details

Message ID 1515571815-24627-1-git-send-email-zhong.li@intel.com
State New
Headers show

Commit Message

Zhong Li Jan. 10, 2018, 8:10 a.m.
Currently a hacky way is used for some specific codecs such as
H264/VP6F/DXV.
Replace with a more generic way(an evolution based on a history commit
9de9b828ef005dec37052548c195a6b4f18fc701 but reverted somehow) to handle these cases.

Signed-off-by: Zhong Li <zhong.li@intel.com>
---
 libavcodec/utils.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Hendrik Leppkes Jan. 10, 2018, 9:08 a.m.
On Wed, Jan 10, 2018 at 9:10 AM, Zhong Li <zhong.li@intel.com> wrote:
> Currently a hacky way is used for some specific codecs such as
> H264/VP6F/DXV.
> Replace with a more generic way(an evolution based on a history commit
> 9de9b828ef005dec37052548c195a6b4f18fc701 but reverted somehow) to handle these cases.
>

What does this actually fix or change? That should preferably be
documented in the commit message.

- Hendrik
Carl Eugen Hoyos Jan. 10, 2018, 2:18 p.m.
2018-01-10 9:10 GMT+01:00 Zhong Li <zhong.li@intel.com>:
> Currently a hacky way is used for some specific codecs such as
> H264/VP6F/DXV.

> Replace with a more generic way(an evolution based on a history commit
> 9de9b828ef005dec37052548c195a6b4f18fc701 but reverted somehow)

This commit was never part of FFmpeg.

> to handle these cases.

Carl Eugen
Zhong Li Jan. 11, 2018, 2:01 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Carl Eugen Hoyos

> Sent: Wednesday, January 10, 2018 10:18 PM

> To: FFmpeg development discussions and patches

> <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH] Don't overwrite previously setup

> dimensions for all codecs

> 

> 2018-01-10 9:10 GMT+01:00 Zhong Li <zhong.li@intel.com>:

> > Currently a hacky way is used for some specific codecs such as

> > H264/VP6F/DXV.

> 

> > Replace with a more generic way(an evolution based on a history commit

> > 9de9b828ef005dec37052548c195a6b4f18fc701 but reverted somehow)

> 

> This commit was never part of FFmpeg.


IIUC, This commit was merged from Libav, checking the commit log of cf7d3846fc865df47bd64f7d4d10bbedf9e3a17b:
   
    Merge commit '9de9b828ef005dec37052548c195a6b4f18fc701'

    * commit '9de9b828ef005dec37052548c195a6b4f18fc701':
      lavc: don't overwrite display dimensions with coded dimensions.
      lavc: extend / update the [coded_]{width,height} doxy

    Conflicts:
        libavcodec/avcodec.h
        libavcodec/utils.c

    The change to the w/h handling is not merged as it breaks lowres

    Merged-by: Michael Niedermayer <michaelni@gmx.at>
wm4 Jan. 11, 2018, 2:08 a.m.
On Thu, 11 Jan 2018 02:01:38 +0000
"Li, Zhong" <zhong.li@intel.com> wrote:

> IIUC, This commit was merged from Libav, checking the commit log of cf7d3846fc865df47bd64f7d4d10bbedf9e3a17b:

Yeah, but...

>     Merge commit '9de9b828ef005dec37052548c195a6b4f18fc701'
> 
>     * commit '9de9b828ef005dec37052548c195a6b4f18fc701':
>       lavc: don't overwrite display dimensions with coded dimensions.
>       lavc: extend / update the [coded_]{width,height} doxy
> 
>     Conflicts:
>         libavcodec/avcodec.h
>         libavcodec/utils.c
> 
>     The change to the w/h handling is not merged as it breaks lowres
> 
>     Merged-by: Michael Niedermayer <michaelni@gmx.at>

This says this specific part was skipped. Apparently it broke lowres,
an obscure feature that probably nobody uses and that Libav removed.
Zhong Li Jan. 11, 2018, 5:34 a.m.
> > IIUC, This commit was merged from Libav, checking the commit log of

> cf7d3846fc865df47bd64f7d4d10bbedf9e3a17b:

> 

> Yeah, but...

> 

> >     Merge commit '9de9b828ef005dec37052548c195a6b4f18fc701'

> >

> >     * commit '9de9b828ef005dec37052548c195a6b4f18fc701':

> >       lavc: don't overwrite display dimensions with coded dimensions.

> >       lavc: extend / update the [coded_]{width,height} doxy

> >

> >     Conflicts:

> >         libavcodec/avcodec.h

> >         libavcodec/utils.c

> >

> >     The change to the w/h handling is not merged as it breaks lowres

> >

> >     Merged-by: Michael Niedermayer <michaelni@gmx.at>

> 

> This says this specific part was skipped. Apparently it broke lowres, an

> obscure feature that probably nobody uses and that Libav removed.


Yup... 9de9b82 was not merged as a whole. 
I guess I am not the only one person who is confused why w/h are overwritten by coded_w/coded_h here (https://ffmpeg.org/pipermail/ffmpeg-devel/2013-March/140142.html )
For "lowres", is it still broken for H264/VP6F/DXV by current code?
wm4 Jan. 11, 2018, 5:41 a.m.
On Thu, 11 Jan 2018 05:34:07 +0000
"Li, Zhong" <zhong.li@intel.com> wrote:

> > > IIUC, This commit was merged from Libav, checking the commit log of  
> > cf7d3846fc865df47bd64f7d4d10bbedf9e3a17b:
> > 
> > Yeah, but...
> >   
> > >     Merge commit '9de9b828ef005dec37052548c195a6b4f18fc701'
> > >
> > >     * commit '9de9b828ef005dec37052548c195a6b4f18fc701':
> > >       lavc: don't overwrite display dimensions with coded dimensions.
> > >       lavc: extend / update the [coded_]{width,height} doxy
> > >
> > >     Conflicts:
> > >         libavcodec/avcodec.h
> > >         libavcodec/utils.c
> > >
> > >     The change to the w/h handling is not merged as it breaks lowres
> > >
> > >     Merged-by: Michael Niedermayer <michaelni@gmx.at>  
> > 
> > This says this specific part was skipped. Apparently it broke lowres, an
> > obscure feature that probably nobody uses and that Libav removed.  
> 
> Yup... 9de9b82 was not merged as a whole. 
> I guess I am not the only one person who is confused why w/h are overwritten by coded_w/coded_h here (https://ffmpeg.org/pipermail/ffmpeg-devel/2013-March/140142.html )
> For "lowres", is it still broken for H264/VP6F/DXV by current code?

I don't know (from the mail above this might be more an API problem?),
but I'd prefer your patch over the current situation as well.
Carl Eugen Hoyos Jan. 11, 2018, 5:46 a.m.
2018-01-11 3:01 GMT+01:00 Li, Zhong <zhong.li@intel.com>:
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
>> Of Carl Eugen Hoyos
>> Sent: Wednesday, January 10, 2018 10:18 PM
>> To: FFmpeg development discussions and patches
>> <ffmpeg-devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH] Don't overwrite previously setup
>> dimensions for all codecs
>>
>> 2018-01-10 9:10 GMT+01:00 Zhong Li <zhong.li@intel.com>:
>> > Currently a hacky way is used for some specific codecs such as
>> > H264/VP6F/DXV.
>>
>> > Replace with a more generic way(an evolution based on a history commit
>> > 9de9b828ef005dec37052548c195a6b4f18fc701 but reverted somehow)
>>
>> This commit was never part of FFmpeg.
>
> IIUC, This commit was merged from Libav

But not the part you referenced in your original mail here.

Would you like to also comment on Hendrik's question?

Carl Eugen
Zhong Li Jan. 12, 2018, 2:40 a.m.
> >> 2018-01-10 9:10 GMT+01:00 Zhong Li <zhong.li@intel.com>:

> >> > Currently a hacky way is used for some specific codecs such as

> >> > H264/VP6F/DXV.

> >>

> >> > Replace with a more generic way(an evolution based on a history

> >> > commit

> >> > 9de9b828ef005dec37052548c195a6b4f18fc701 but reverted somehow)

> >>

> >> This commit was never part of FFmpeg.

> >

> > IIUC, This commit was merged from Libav

> 

> But not the part you referenced in your original mail here.

> 

> Would you like to also comment on Hendrik's question?


I'm not sure you are interested to an issue on a forked repo (https://github.com/Intel-FFmpeg-Plugin/Intel_FFmpeg_plugins/issues/12 ).
This issue is caused by w/h are overwritten by coded_w/coded_h (Just like https://trac.ffmpeg.org/ticket/1386 ). 
It can't be reproduced on latest FFmpeg since codec->coded_w/coded_h haven't been passed in avcodec_parameters_from_context(). It means ticket #1386 also can't be reproduced now even remove the original fixing patch (commit 33b0549)
But latest FFmpeg introduces a new issue (https://trac.ffmpeg.org/ticket/6958 )

Patch hide | download patch | download mbox

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 4d736d2..3f3f3db 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -684,16 +684,12 @@  int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
         goto free_and_end;
     }
 
-    // only call ff_set_dimensions() for non H.264/VP6F/DXV codecs so as not to overwrite previously setup dimensions
-    if (!(avctx->coded_width && avctx->coded_height && avctx->width && avctx->height &&
-          (avctx->codec_id == AV_CODEC_ID_H264 || avctx->codec_id == AV_CODEC_ID_VP6F || avctx->codec_id == AV_CODEC_ID_DXV))) {
-    if (avctx->coded_width && avctx->coded_height)
+    if (avctx->coded_width && avctx->coded_height && !avctx->width && !avctx->height)
         ret = ff_set_dimensions(avctx, avctx->coded_width, avctx->coded_height);
-    else if (avctx->width && avctx->height)
+    else if (avctx->width && avctx->height && !avctx->coded_width && !avctx->coded_height)
         ret = ff_set_dimensions(avctx, avctx->width, avctx->height);
     if (ret < 0)
         goto free_and_end;
-    }
 
     if ((avctx->coded_width || avctx->coded_height || avctx->width || avctx->height)
         && (  av_image_check_size2(avctx->coded_width, avctx->coded_height, avctx->max_pixels, AV_PIX_FMT_NONE, 0, avctx) < 0