| Message ID | 20170219172307.27012-2-sw@jkqxz.net |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Sun, Feb 19, 2017 at 12:23 PM, Mark Thompson <sw@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. Ronald
On 19/02/17 21:04, Ronald S. Bultje wrote: > Hi, > > On Sun, Feb 19, 2017 at 12:23 PM, Mark Thompson <sw@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? - Mark
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@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 ? [...]
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@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/. 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. 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. - Mark
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@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. > > 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 ? 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. am i missing something ? is what i suggested difficult to do or do you see a issue with that solution ? thx [...]
On Mon, Feb 20, 2017 at 11:44 AM, Michael Niedermayer <michael@niedermayer.cc> 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@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. > > >> >> 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 ? > 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. > > am i missing something ? > is what i suggested difficult to do or do you see a issue with that > solution ? > The entire WebP alpha concept is a huge hack, because the vp8 bitstream or vp8 decoder know nothing of the alpha, its coded separately. However, to make room to store the alpha, the vp8 decoder needs to be told to decode into an alpha-enabled format, so that the webp decoder can fill it. The vp8 decoder can't know its supposed to do this, unless someone tells it externally - which is where get_format comes in. - Hendrik
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@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. - Mark
On Mon, 20 Feb 2017 11:20:48 +0000 Mark Thompson <sw@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@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?
On Mon, Feb 20, 2017 at 12:28:55PM +0100, wm4 wrote: > On Mon, 20 Feb 2017 11:20:48 +0000 > Mark Thompson <sw@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@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 [...]
diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c index c1c3eb7072..365912cc88 100644 --- a/libavcodec/vp8.c +++ b/libavcodec/vp8.c @@ -72,16 +72,30 @@ static int vp8_alloc_frame(VP8Context *s, VP8Frame *f, int ref) if ((ret = ff_thread_get_buffer(s->avctx, &f->tf, ref ? AV_GET_BUFFER_FLAG_REF : 0)) < 0) return ret; - if (!(f->seg_map = av_buffer_allocz(s->mb_width * s->mb_height))) { - ff_thread_release_buffer(s->avctx, &f->tf); - return AVERROR(ENOMEM); + if (!(f->seg_map = av_buffer_allocz(s->mb_width * s->mb_height))) + goto fail; + if (s->avctx->hwaccel) { + const AVHWAccel *hwaccel = s->avctx->hwaccel; + if (hwaccel->frame_priv_data_size) { + f->hwaccel_priv_buf = av_buffer_allocz(hwaccel->frame_priv_data_size); + if (!f->hwaccel_priv_buf) + goto fail; + f->hwaccel_picture_private = f->hwaccel_priv_buf->data; + } } return 0; + +fail: + av_buffer_unref(&f->seg_map); + ff_thread_release_buffer(s->avctx, &f->tf); + return AVERROR(ENOMEM); } static void vp8_release_frame(VP8Context *s, VP8Frame *f) { av_buffer_unref(&f->seg_map); + av_buffer_unref(&f->hwaccel_priv_buf); + f->hwaccel_picture_private = NULL; ff_thread_release_buffer(s->avctx, &f->tf); } @@ -99,6 +113,12 @@ static int vp8_ref_frame(VP8Context *s, VP8Frame *dst, VP8Frame *src) vp8_release_frame(s, dst); return AVERROR(ENOMEM); } + if (src->hwaccel_picture_private) { + dst->hwaccel_priv_buf = av_buffer_ref(src->hwaccel_priv_buf); + if (!dst->hwaccel_priv_buf) + return AVERROR(ENOMEM); + dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data; + } return 0; } @@ -140,7 +160,7 @@ static VP8Frame *vp8_find_free_buffer(VP8Context *s) av_log(s->avctx, AV_LOG_FATAL, "Ran out of free frames!\n"); abort(); } - if (frame->tf.f->data[0]) + if (frame->tf.f->buf[0]) vp8_release_frame(s, frame); return frame; @@ -218,8 +238,9 @@ static void parse_segment_info(VP8Context *s) int i; s->segmentation.update_map = vp8_rac_get(c); + s->segmentation.update_feature_data = vp8_rac_get(c); - if (vp8_rac_get(c)) { // update segment feature data + if (s->segmentation.update_feature_data) { s->segmentation.absolute_vals = vp8_rac_get(c); for (i = 0; i < 4; i++) @@ -273,11 +294,14 @@ static int setup_partitions(VP8Context *s, const uint8_t *buf, int buf_size) int size = AV_RL24(sizes + 3 * i); if (buf_size - size < 0) return -1; + s->coeff_partition_size[i] = size; ff_vp56_init_range_decoder(&s->coeff_partition[i], buf, size); buf += size; buf_size -= size; } + + s->coeff_partition_size[i] = buf_size; ff_vp56_init_range_decoder(&s->coeff_partition[i], buf, buf_size); return 0; @@ -307,28 +331,28 @@ static void vp8_get_quants(VP8Context *s) VP56RangeCoder *c = &s->c; int i, base_qi; - int yac_qi = vp8_rac_get_uint(c, 7); - int ydc_delta = vp8_rac_get_sint(c, 4); - int y2dc_delta = vp8_rac_get_sint(c, 4); - int y2ac_delta = vp8_rac_get_sint(c, 4); - int uvdc_delta = vp8_rac_get_sint(c, 4); - int uvac_delta = vp8_rac_get_sint(c, 4); + s->quant.yac_qi = vp8_rac_get_uint(c, 7); + s->quant.ydc_delta = vp8_rac_get_sint(c, 4); + s->quant.y2dc_delta = vp8_rac_get_sint(c, 4); + s->quant.y2ac_delta = vp8_rac_get_sint(c, 4); + s->quant.uvdc_delta = vp8_rac_get_sint(c, 4); + s->quant.uvac_delta = vp8_rac_get_sint(c, 4); for (i = 0; i < 4; i++) { if (s->segmentation.enabled) { base_qi = s->segmentation.base_quant[i]; if (!s->segmentation.absolute_vals) - base_qi += yac_qi; + base_qi += s->quant.yac_qi; } else - base_qi = yac_qi; + base_qi = s->quant.yac_qi; - s->qmat[i].luma_qmul[0] = vp8_dc_qlookup[av_clip_uintp2(base_qi + ydc_delta, 7)]; + s->qmat[i].luma_qmul[0] = vp8_dc_qlookup[av_clip_uintp2(base_qi + s->quant.ydc_delta, 7)]; s->qmat[i].luma_qmul[1] = vp8_ac_qlookup[av_clip_uintp2(base_qi, 7)]; - s->qmat[i].luma_dc_qmul[0] = vp8_dc_qlookup[av_clip_uintp2(base_qi + y2dc_delta, 7)] * 2; + s->qmat[i].luma_dc_qmul[0] = vp8_dc_qlookup[av_clip_uintp2(base_qi + s->quant.y2dc_delta, 7)] * 2; /* 101581>>16 is equivalent to 155/100 */ - s->qmat[i].luma_dc_qmul[1] = vp8_ac_qlookup[av_clip_uintp2(base_qi + y2ac_delta, 7)] * 101581 >> 16; - s->qmat[i].chroma_qmul[0] = vp8_dc_qlookup[av_clip_uintp2(base_qi + uvdc_delta, 7)]; - s->qmat[i].chroma_qmul[1] = vp8_ac_qlookup[av_clip_uintp2(base_qi + uvac_delta, 7)]; + s->qmat[i].luma_dc_qmul[1] = vp8_ac_qlookup[av_clip_uintp2(base_qi + s->quant.y2ac_delta, 7)] * 101581 >> 16; + s->qmat[i].chroma_qmul[0] = vp8_dc_qlookup[av_clip_uintp2(base_qi + s->quant.uvdc_delta, 7)]; + s->qmat[i].chroma_qmul[1] = vp8_ac_qlookup[av_clip_uintp2(base_qi + s->quant.uvac_delta, 7)]; s->qmat[i].luma_dc_qmul[1] = FFMAX(s->qmat[i].luma_dc_qmul[1], 8); s->qmat[i].chroma_qmul[0] = FFMIN(s->qmat[i].chroma_qmul[0], 132); @@ -656,6 +680,8 @@ static int vp8_decode_frame_header(VP8Context *s, const uint8_t *buf, int buf_si buf += 3; buf_size -= 3; + s->header_partition_size = header_size; + if (s->profile > 3) av_log(s->avctx, AV_LOG_WARNING, "Unknown profile %d\n", s->profile); @@ -719,9 +745,11 @@ static int vp8_decode_frame_header(VP8Context *s, const uint8_t *buf, int buf_si s->filter.level = vp8_rac_get_uint(c, 6); s->filter.sharpness = vp8_rac_get_uint(c, 3); - if ((s->lf_delta.enabled = vp8_rac_get(c))) - if (vp8_rac_get(c)) + if ((s->lf_delta.enabled = vp8_rac_get(c))) { + s->lf_delta.update = vp8_rac_get(c); + if (s->lf_delta.update) update_lf_deltas(s); + } if (setup_partitions(s, buf, buf_size)) { av_log(s->avctx, AV_LOG_ERROR, "Invalid partitions\n"); @@ -761,6 +789,13 @@ static int vp8_decode_frame_header(VP8Context *s, const uint8_t *buf, int buf_si vp78_update_pred16x16_pred8x8_mvc_probabilities(s, VP8_MVC_SIZE); } + // Record the entropy coder state here so that hwaccels can use it. + s->c.code_word = vp56_rac_renorm(&s->c); + s->coder_state_at_header_end.input = s->c.buffer - (-s->c.bits / 8); + s->coder_state_at_header_end.range = s->c.high; + s->coder_state_at_header_end.value = s->c.code_word >> 16; + s->coder_state_at_header_end.bit_count = -s->c.bits % 8; + return 0; } @@ -2521,7 +2556,6 @@ static int vp8_decode_mb_row_sliced(AVCodecContext *avctx, void *tdata, return vp78_decode_mb_row_sliced(avctx, tdata, jobnr, threadnr, IS_VP8); } - static av_always_inline int vp78_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPacket *avpkt, int is_vp7) @@ -2539,6 +2573,20 @@ int vp78_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, if (ret < 0) goto err; + if (!is_vp7 && s->pix_fmt == AV_PIX_FMT_NONE) { + enum AVPixelFormat pix_fmts[] = { + AV_PIX_FMT_YUV420P, + AV_PIX_FMT_NONE, + }; + + s->pix_fmt = ff_get_format(s->avctx, pix_fmts); + if (s->pix_fmt < 0) { + ret = AVERROR(EINVAL); + goto err; + } + avctx->pix_fmt = s->pix_fmt; + } + prev_frame = s->framep[VP56_FRAME_CURRENT]; referenced = s->update_last || s->update_golden == VP56_FRAME_CURRENT || @@ -2557,7 +2605,7 @@ int vp78_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, // release no longer referenced frames for (i = 0; i < 5; i++) - if (s->frames[i].tf.f->data[0] && + if (s->frames[i].tf.f->buf[0] && &s->frames[i] != prev_frame && &s->frames[i] != s->framep[VP56_FRAME_PREVIOUS] && &s->frames[i] != s->framep[VP56_FRAME_GOLDEN] && @@ -2610,54 +2658,69 @@ int vp78_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, s->next_framep[VP56_FRAME_CURRENT] = curframe; - if (avctx->codec->update_thread_context) - ff_thread_finish_setup(avctx); + ff_thread_finish_setup(avctx); - s->linesize = curframe->tf.f->linesize[0]; - s->uvlinesize = curframe->tf.f->linesize[1]; + if (avctx->hwaccel) { + ret = avctx->hwaccel->start_frame(avctx, avpkt->data, avpkt->size); + if (ret < 0) + goto err; - memset(s->top_nnz, 0, s->mb_width * sizeof(*s->top_nnz)); - /* Zero macroblock structures for top/top-left prediction - * from outside the frame. */ - if (!s->mb_layout) - memset(s->macroblocks + s->mb_height * 2 - 1, 0, - (s->mb_width + 1) * sizeof(*s->macroblocks)); - if (!s->mb_layout && s->keyframe) - memset(s->intra4x4_pred_mode_top, DC_PRED, s->mb_width * 4); + ret = avctx->hwaccel->decode_slice(avctx, avpkt->data, avpkt->size); + if (ret < 0) + goto err; - memset(s->ref_count, 0, sizeof(s->ref_count)); + ret = avctx->hwaccel->end_frame(avctx); + if (ret < 0) + goto err; - if (s->mb_layout == 1) { - // Make sure the previous frame has read its segmentation map, - // if we re-use the same map. - if (prev_frame && s->segmentation.enabled && - !s->segmentation.update_map) - ff_thread_await_progress(&prev_frame->tf, 1, 0); - if (is_vp7) - vp7_decode_mv_mb_modes(avctx, curframe, prev_frame); + } else { + s->linesize = curframe->tf.f->linesize[0]; + s->uvlinesize = curframe->tf.f->linesize[1]; + + memset(s->top_nnz, 0, s->mb_width * sizeof(*s->top_nnz)); + /* Zero macroblock structures for top/top-left prediction + * from outside the frame. */ + if (!s->mb_layout) + memset(s->macroblocks + s->mb_height * 2 - 1, 0, + (s->mb_width + 1) * sizeof(*s->macroblocks)); + if (!s->mb_layout && s->keyframe) + memset(s->intra4x4_pred_mode_top, DC_PRED, s->mb_width * 4); + + memset(s->ref_count, 0, sizeof(s->ref_count)); + + if (s->mb_layout == 1) { + // Make sure the previous frame has read its segmentation map, + // if we re-use the same map. + if (prev_frame && s->segmentation.enabled && + !s->segmentation.update_map) + ff_thread_await_progress(&prev_frame->tf, 1, 0); + if (is_vp7) + vp7_decode_mv_mb_modes(avctx, curframe, prev_frame); + else + vp8_decode_mv_mb_modes(avctx, curframe, prev_frame); + } + + if (avctx->active_thread_type == FF_THREAD_FRAME) + num_jobs = 1; else - vp8_decode_mv_mb_modes(avctx, curframe, prev_frame); - } + num_jobs = FFMIN(s->num_coeff_partitions, avctx->thread_count); + s->num_jobs = num_jobs; + s->curframe = curframe; + s->prev_frame = prev_frame; + s->mv_min.y = -MARGIN; + s->mv_max.y = ((s->mb_height - 1) << 6) + MARGIN; + for (i = 0; i < MAX_THREADS; i++) { + s->thread_data[i].thread_mb_pos = 0; + s->thread_data[i].wait_mb_pos = INT_MAX; + } - if (avctx->active_thread_type == FF_THREAD_FRAME) - num_jobs = 1; - else - num_jobs = FFMIN(s->num_coeff_partitions, avctx->thread_count); - s->num_jobs = num_jobs; - s->curframe = curframe; - s->prev_frame = prev_frame; - s->mv_min.y = -MARGIN; - s->mv_max.y = ((s->mb_height - 1) << 6) + MARGIN; - for (i = 0; i < MAX_THREADS; i++) { - s->thread_data[i].thread_mb_pos = 0; - s->thread_data[i].wait_mb_pos = INT_MAX; + if (is_vp7) + avctx->execute2(avctx, vp7_decode_mb_row_sliced, s->thread_data, NULL, + num_jobs); + else + avctx->execute2(avctx, vp8_decode_mb_row_sliced, s->thread_data, NULL, + num_jobs); } - if (is_vp7) - avctx->execute2(avctx, vp7_decode_mb_row_sliced, s->thread_data, NULL, - num_jobs); - else - avctx->execute2(avctx, vp8_decode_mb_row_sliced, s->thread_data, NULL, - num_jobs); ff_thread_report_progress(&curframe->tf, INT_MAX, 0); memcpy(&s->framep[0], &s->next_framep[0], sizeof(s->framep[0]) * 4); @@ -2728,6 +2791,7 @@ int vp78_decode_init(AVCodecContext *avctx, int is_vp7) s->avctx = avctx; s->vp7 = avctx->codec->id == AV_CODEC_ID_VP7; + s->pix_fmt = AV_PIX_FMT_NONE; avctx->pix_fmt = AV_PIX_FMT_YUV420P; avctx->internal->allocate_progress = 1; @@ -2801,13 +2865,14 @@ static int vp8_decode_update_thread_context(AVCodecContext *dst, s->mb_height = s_src->mb_height; } + s->pix_fmt = s_src->pix_fmt; s->prob[0] = s_src->prob[!s_src->update_probabilities]; s->segmentation = s_src->segmentation; s->lf_delta = s_src->lf_delta; memcpy(s->sign_bias, s_src->sign_bias, sizeof(s->sign_bias)); for (i = 0; i < FF_ARRAY_ELEMS(s_src->frames); i++) { - if (s_src->frames[i].tf.f->data[0]) { + if (s_src->frames[i].tf.f->buf[0]) { int ret = vp8_ref_frame(s, &s->frames[i], &s_src->frames[i]); if (ret < 0) return ret; diff --git a/libavcodec/vp8.h b/libavcodec/vp8.h index 374e1388e2..63f6c3147f 100644 --- a/libavcodec/vp8.h +++ b/libavcodec/vp8.h @@ -125,6 +125,9 @@ typedef struct VP8ThreadData { typedef struct VP8Frame { ThreadFrame tf; AVBufferRef *seg_map; + + AVBufferRef *hwaccel_priv_buf; + void *hwaccel_picture_private; } VP8Frame; typedef struct VP8intmv { @@ -136,6 +139,8 @@ typedef struct VP8intmv { typedef struct VP8Context { VP8ThreadData *thread_data; AVCodecContext *avctx; + enum AVPixelFormat pix_fmt; + VP8Frame *framep[4]; VP8Frame *next_framep[4]; VP8Frame *curframe; @@ -165,6 +170,7 @@ typedef struct VP8Context { uint8_t enabled; uint8_t absolute_vals; uint8_t update_map; + uint8_t update_feature_data; int8_t base_quant[4]; int8_t filter_level[4]; ///< base loop filter level } segmentation; @@ -192,8 +198,19 @@ typedef struct VP8Context { int16_t chroma_qmul[2]; } qmat[4]; + // Raw quantisation values, which may be needed by hwaccel decode. + struct { + int yac_qi; + int ydc_delta; + int y2dc_delta; + int y2ac_delta; + int uvdc_delta; + int uvac_delta; + } quant; + struct { uint8_t enabled; ///< whether each mb can have a different strength based on mode/ref + uint8_t update; /** * filter strength adjustment for the following macroblock modes: @@ -221,6 +238,20 @@ typedef struct VP8Context { VP56RangeCoder c; ///< header context, includes mb modes and motion vectors + /* This contains the entropy coder state at the end of the header + * block, in the form specified by the standard. For use by + * hwaccels, so that a hardware decoder has the information to + * start decoding at the macroblock layer. + */ + struct { + const uint8_t *input; + uint32_t range; + uint32_t value; + int bit_count; + } coder_state_at_header_end; + + int header_partition_size; + /** * These are all of the updatable probabilities for binary decisions. * They are only implicitly reset on keyframes, making it quite likely @@ -258,6 +289,7 @@ typedef struct VP8Context { */ int num_coeff_partitions; VP56RangeCoder coeff_partition[8]; + int coeff_partition_size[8]; VideoDSPContext vdsp; VP8DSPContext vp8dsp; H264PredContext hpc; diff --git a/libavcodec/webp.c b/libavcodec/webp.c index 45abfdc3ca..a9b9136153 100644 --- a/libavcodec/webp.c +++ b/libavcodec/webp.c @@ -1316,6 +1316,16 @@ static int vp8_lossy_decode_alpha(AVCodecContext *avctx, AVFrame *p, return 0; } +static enum AVPixelFormat webp_get_format(AVCodecContext *avctx, + const enum AVPixelFormat *formats) +{ + WebPContext *s = avctx->priv_data; + if (s->has_alpha) + return AV_PIX_FMT_YUVA420P; + else + return AV_PIX_FMT_YUV420P; +} + static int vp8_lossy_decode_frame(AVCodecContext *avctx, AVFrame *p, int *got_frame, uint8_t *data_start, unsigned int data_size) @@ -1327,8 +1337,7 @@ static int vp8_lossy_decode_frame(AVCodecContext *avctx, AVFrame *p, if (!s->initialized) { ff_vp8_decode_init(avctx); s->initialized = 1; - if (s->has_alpha) - avctx->pix_fmt = AV_PIX_FMT_YUVA420P; + avctx->get_format = webp_get_format; } s->lossless = 0;