On Mon, Feb 20, 2017 at 12:28:55PM +0100, wm4 wrote: > On Mon, 20 Feb 2017 11:20:48 +0000 > Mark Thompson <s...@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 <s...@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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
