[FFmpeg-devel] [PATCH 1/4] vp8: Add hwaccel hooks

[Michael Niedermayer]

On Mon, Feb 20, 2017 at 12:28:55PM +0100, wm4 wrote:
> On Mon, 20 Feb 2017 11:20:48 +0000
> Mark Thompson <sw at jkqxz.net> wrote:
> 
> > On 20/02/17 10:44, Michael Niedermayer wrote:
> > > On Mon, Feb 20, 2017 at 10:23:27AM +0000, Mark Thompson wrote:  
> > >> On 20/02/17 02:35, Michael Niedermayer wrote:  
> > >>> On Sun, Feb 19, 2017 at 09:29:33PM +0000, Mark Thompson wrote:  
> > >>>> On 19/02/17 21:04, Ronald S. Bultje wrote:  
> > >>>>> Hi,
> > >>>>>
> > >>>>> On Sun, Feb 19, 2017 at 12:23 PM, Mark Thompson <sw at jkqxz.net> wrote:
> > >>>>>  
> > >>>>>> diff --git a/libavcodec/webp.c b/libavcodec/webp.c
> > >>>>>>  
> > >>>>> [..]
> > >>>>>  
> > >>>>>> +        avctx->get_format = webp_get_format;  
> > >>>>>
> > >>>>>
> > >>>>> Docs say:
> > >>>>> "decoding: Set by user, if not set the native format will be chosen."
> > >>>>> So I don't think decoders are supposed to set this.  
> > >>>>
> > >>>> The webp decoder does not support get_format.  I suppose the user could technically store something there and then read it back, so save and restore the value across the relevant calls?  
> > >>>
> > >>> This is quite ugly, why do you want to do that ?
> > >>>
> > >>> get_format is set by the user
> > >>> the get_format API requires the function to choose one of the caller
> > >>> provided formats and it can choose any.
> > >>> I dont know what your function does but its different from the API.
> > >>> It smells very much like a hack ...
> > >>>
> > >>> The one situation in which you can set get_format from libavcodec is
> > >>> when there is a seperate codec context you created, that is that you
> > >>> are its user.
> > >>>
> > >>> can you explain why this code messes with avctx->get_format ?
> > >>> and for example doesnt change the code that calls get_format so that
> > >>> it passes a correct pixel format list which then by any get_format()
> > >>> results in a correct format ?
> > >>> or am i missing something ?  
> > >>  
> > >   
> > >> The current WebP decoder calls the VP8 decoder /using the same AVCodecContext/.  
> > > 
> > > so they are kind of the same decoder
> > > 
> > >   
> > >> Previously the format was set directly on the VP8Context, but now that the VP8 decoder uses ff_get_format() that initial value is not used.  This change adds a get_format() callback on the AVCodecContext used by the VP8 decoder to set the format appropriately.  
> > > 
> > > But this is semantically wrong, the formats supported by the decoder
> > > implementation are choosen by the code calling get_format().
> > > the get_format callback chooses amongth these formats depending on the
> > > users preferrance/usefullness.  
> > 
> > Yes.  It's a hack to make the VP8 decoder allocate a non-native frame format (YUVA420P) which is compatible with its native format (YUV420P) for any use it makes of it.  This is a hack which already exists, we are just moving it to a different place now that the VP8 decoder uses ff_get_format().
> > 
> > >> Now, because that is the same AVCodecContext as the one supplied by the user, the user could themselves have stored something into the get_format field (even though it isn't used by the WebP decoder) and expect to be able to read it back correctly later.  I think this would be madness, but is I suppose technically allowed; saving and restoring the value across the VP8 calls would fix that case.  
> > > 
> > > Why do you try to misuse the API ?  
> > 
> > To make the minimal modifications to existing hacks for fate to pass for WebP.  I do not care at all about WebP, but the existing hacks abusing the VP8 decoder block the other changes.
> > 
> > > i mean i think we agree that this violates the API. Making sure it
> > > works doesnt solve that it violates the API. And anyone working on
> > > the API would likely assume that code conforms to the API documentation.  
> > > -> the developer would think one thing and the code would do another  
> > > thats a recipe for creating bugs.
> > > 
> > > I think the code calling *get_format() should pass the correct list
> > > of formats to the callback and not some other part override the users
> > > get_format calback to work around that the list passed is wrong.  
> > 
> > The user's get_format callback is never used.  (There is only one output format possibility for each input, so it wouldn't do anything anyway.)
> > 
> > > am i missing something ?
> > > is what i suggested difficult to do or do you see a issue with that
> > > solution ?  
> > 
> > The list passed by the VP8 decoder to ff_get_format() is not wrong - the VP8 decoder does not support alpha so including YUVA420P would be incorrect.
> > 
> 
> Would it be possible to add some field to the private context which
> controls this instead?

thats what i was thinking of as well

thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170220/2d6b6e1d/attachment.sig>