Re: [libav-devel] [PATCH 1/3] vp8: Add hwaccel hooks

[Mark Thompson]

On 08/09/16 11:33, Hendrik Leppkes wrote:
> On Thu, Sep 8, 2016 at 7:17 AM, Anton Khirnov  wrote:
>> Quoting Mark Thompson (2016-09-06 22:53:18)
>>> @@ -2480,6 +2514,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_BUG;
>>
>> AVERROR_BUG is meant to be used in cases that should not happen in valid
>> code, it's like a "soft assert".
>> Since it's perfectly valid for ff_get_format() to fail, you should
>> instead use something like AVERROR_UNKNOWN.
> 
> Doesn't ff_get_format return an error code which he could/should just forward?

No?  ff_get_format() doesn't return any error codes directly.  If the
get_format() callback returns one then that is a bug because it gets assigned to
an enum AVPixelFormat (which is signed and need not be able to represent all of
int, hence undefined behaviour).

- Mark

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/3] vp8: Add hwaccel hooks

[Hendrik Leppkes]

On Thu, Sep 8, 2016 at 7:17 AM, Anton Khirnov  wrote:
> Quoting Mark Thompson (2016-09-06 22:53:18)
>> @@ -2480,6 +2514,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_BUG;
>
> AVERROR_BUG is meant to be used in cases that should not happen in valid
> code, it's like a "soft assert".
> Since it's perfectly valid for ff_get_format() to fail, you should
> instead use something like AVERROR_UNKNOWN.

Doesn't ff_get_format return an error code which he could/should just forward?

- Hendrik
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/3] vp8: Add hwaccel hooks

[Anton Khirnov]

Quoting Mark Thompson (2016-09-06 22:53:18)
> @@ -2480,6 +2514,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_BUG;

AVERROR_BUG is meant to be used in cases that should not happen in valid
code, it's like a "soft assert".
Since it's perfectly valid for ff_get_format() to fail, you should
instead use something like AVERROR_UNKNOWN.

The rest of the set looks fine to me, feel free to push with the above
change.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH 1/3] vp8: Add hwaccel hooks

[Mark Thompson]

Also adds some extra fields to the main context structure that may
be needed by a hwaccel decoder.
---

v2: review comments incorporated.

On 06/09/16 09:57, Anton Khirnov wrote:
> Quoting Mark Thompson (2016-09-04 14:43:21)
>> @@ -313,13 +336,19 @@ static void get_quants(VP8Context *s)
>>  } else
>>  base_qi = yac_qi;
>>
>> -s->qmat[i].luma_qmul[0]= vp8_dc_qlookup[av_clip_uintp2(base_qi 
>> + 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_qmul[0]= vp8_dc_qlookup[s->qmat_raw[i][1] =
>> +av_clip_uintp2(base_qi 
>> + ydc_delta,  7)];
>> +s->qmat[i].luma_qmul[1]= vp8_ac_qlookup[s->qmat_raw[i][0] =
>> +av_clip_uintp2(base_qi, 
>>  7)];
>> +s->qmat[i].luma_dc_qmul[0] = vp8_dc_qlookup[s->qmat_raw[i][2] =
>> +av_clip_uintp2(base_qi 
>> + 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[s->qmat_raw[i][3] =
>> +av_clip_uintp2(base_qi 
>> + y2ac_delta, 7)] * 101581 >> 16;
>> +s->qmat[i].chroma_qmul[0]  = vp8_dc_qlookup[s->qmat_raw[i][4] =
>> +av_clip_uintp2(base_qi 
>> + uvdc_delta, 7)];
>> +s->qmat[i].chroma_qmul[1]  = vp8_ac_qlookup[s->qmat_raw[i][5] =
>> +av_clip_uintp2(base_qi 
>> + uvac_delta, 7)];
> 
> Those sneaky assignments are quite nasty, I'd rather see them done
> separately.

I went a bit further and moved the yac_qi, ydc_delta, etc. variables into the 
context structure.  A theoretical other hwaccel might prefer the earlier values 
rather than the partially processed ones.

>> @@ -2666,7 +2750,7 @@ int vp78_decode_init(AVCodecContext *avctx, int is_vp7)
>>  int ret;
>>
>>  s->avctx = avctx;
>> -avctx->pix_fmt = AV_PIX_FMT_YUV420P;
>> +avctx->pix_fmt = AV_PIX_FMT_NONE;
> 
> It's probably better to leave this as is. The initial value of pix_fmt
> before decoding starts is just a hint to the caller about the probable
> pixel format, so they don't have to do expensive decoding to find out
> the actual value.

Changed around somewhat below - if AVCodecContext.pix_fmt has meaning there 
then it can't be used to cache the result, so we need to put it somewhere else.

Thanks,

- Mark


 libavcodec/vp8.c | 185 +--
 libavcodec/vp8.h |  32 ++
 2 files changed, 157 insertions(+), 60 deletions(-)

diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
index 546124c..a3da76f 100644
--- a/libavcodec/vp8.c
+++ b/libavcodec/vp8.c
@@ -64,16 +64,30 @@ static int vp8_alloc_frame(VP8Context *s, VP8Frame *f, int 
ref)
 if ((ret = ff_thread_get_buffer(s->avctx, >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, >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(>seg_map);
+ff_thread_release_buffer(s->avctx, >tf);
+return AVERROR(ENOMEM);
 }

 static void vp8_release_frame(VP8Context *s, VP8Frame *f)
 {
 av_buffer_unref(>seg_map);
+av_buffer_unref(>hwaccel_priv_buf);
+f->hwaccel_picture_private = NULL;
 ff_thread_release_buffer(s->avctx, >tf);
 }

@@ -91,6 +105,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;
 }
@@ 

Re: [libav-devel] [PATCH 1/3] vp8: Add hwaccel hooks

[Anton Khirnov]

Quoting Mark Thompson (2016-09-04 14:43:21)
> Also adds some extra fields to the main context structure that may
> be needed by a hwaccel decoder.
> ---
>  libavcodec/vp8.c | 190 
> +++
>  libavcodec/vp8.h |  25 
>  2 files changed, 162 insertions(+), 53 deletions(-)
> 
> diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
> index 546124c..b825d4c 100644
> --- a/libavcodec/vp8.c
> +++ b/libavcodec/vp8.c
> @@ -64,16 +64,29 @@ static int vp8_alloc_frame(VP8Context *s, VP8Frame *f, 
> int ref)
>  if ((ret = ff_thread_get_buffer(s->avctx, >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, >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;

Doesn't this leak seg_map?

> +f->hwaccel_picture_private = f->hwaccel_priv_buf->data;
> +}
>  }
>  return 0;
> +
> +fail:
> +ff_thread_release_buffer(s->avctx, >tf);
> +return AVERROR(ENOMEM);
>  }
> 
>  static void vp8_release_frame(VP8Context *s, VP8Frame *f)
>  {
>  av_buffer_unref(>seg_map);
> +av_buffer_unref(>hwaccel_priv_buf);
> +f->hwaccel_picture_private = NULL;
>  ff_thread_release_buffer(s->avctx, >tf);
>  }
> 
> @@ -91,6 +104,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;
>  }
> @@ -132,7 +151,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->data[0] || frame->tf.f->buf[0])

Just checking for buf[0] should be enough.

>  vp8_release_frame(s, frame);
> 
>  return frame;
> @@ -209,8 +228,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++)
> @@ -264,11 +284,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(>coeff_partition[i], buf, size);
>  buf  += size;
>  buf_size -= size;
>  }
> +
> +s->coeff_partition_size[i] = buf_size;
>  ff_vp56_init_range_decoder(>coeff_partition[i], buf, buf_size);
> 
>  return 0;
> @@ -313,13 +336,19 @@ static void get_quants(VP8Context *s)
>  } else
>  base_qi = yac_qi;
> 
> -s->qmat[i].luma_qmul[0]= vp8_dc_qlookup[av_clip_uintp2(base_qi + 
> 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_qmul[0]= vp8_dc_qlookup[s->qmat_raw[i][1] =
> +av_clip_uintp2(base_qi + 
> ydc_delta,  7)];
> +s->qmat[i].luma_qmul[1]= vp8_ac_qlookup[s->qmat_raw[i][0] =
> +av_clip_uintp2(base_qi,  
> 7)];
> +s->qmat[i].luma_dc_qmul[0] = vp8_dc_qlookup[s->qmat_raw[i][2] =
> +av_clip_uintp2(base_qi + 
> 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[s->qmat_raw[i][3] =
> +

[libav-devel] [PATCH 1/3] vp8: Add hwaccel hooks

[Mark Thompson]

Also adds some extra fields to the main context structure that may
be needed by a hwaccel decoder.
---
 libavcodec/vp8.c | 190 +++
 libavcodec/vp8.h |  25 
 2 files changed, 162 insertions(+), 53 deletions(-)

diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
index 546124c..b825d4c 100644
--- a/libavcodec/vp8.c
+++ b/libavcodec/vp8.c
@@ -64,16 +64,29 @@ static int vp8_alloc_frame(VP8Context *s, VP8Frame *f, int 
ref)
 if ((ret = ff_thread_get_buffer(s->avctx, >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, >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:
+ff_thread_release_buffer(s->avctx, >tf);
+return AVERROR(ENOMEM);
 }

 static void vp8_release_frame(VP8Context *s, VP8Frame *f)
 {
 av_buffer_unref(>seg_map);
+av_buffer_unref(>hwaccel_priv_buf);
+f->hwaccel_picture_private = NULL;
 ff_thread_release_buffer(s->avctx, >tf);
 }

@@ -91,6 +104,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;
 }
@@ -132,7 +151,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->data[0] || frame->tf.f->buf[0])
 vp8_release_frame(s, frame);

 return frame;
@@ -209,8 +228,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++)
@@ -264,11 +284,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(>coeff_partition[i], buf, size);
 buf  += size;
 buf_size -= size;
 }
+
+s->coeff_partition_size[i] = buf_size;
 ff_vp56_init_range_decoder(>coeff_partition[i], buf, buf_size);

 return 0;
@@ -313,13 +336,19 @@ static void get_quants(VP8Context *s)
 } else
 base_qi = yac_qi;

-s->qmat[i].luma_qmul[0]= vp8_dc_qlookup[av_clip_uintp2(base_qi + 
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_qmul[0]= vp8_dc_qlookup[s->qmat_raw[i][1] =
+av_clip_uintp2(base_qi + 
ydc_delta,  7)];
+s->qmat[i].luma_qmul[1]= vp8_ac_qlookup[s->qmat_raw[i][0] =
+av_clip_uintp2(base_qi,
  7)];
+s->qmat[i].luma_dc_qmul[0] = vp8_dc_qlookup[s->qmat_raw[i][2] =
+av_clip_uintp2(base_qi + 
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[s->qmat_raw[i][3] =
+av_clip_uintp2(base_qi + 
y2ac_delta, 7)] * 101581 >> 16;
+s->qmat[i].chroma_qmul[0]  = vp8_dc_qlookup[s->qmat_raw[i][4] =
+av_clip_uintp2(base_qi + 
uvdc_delta, 7)];
+s->qmat[i].chroma_qmul[1]  = vp8_ac_qlookup[s->qmat_raw[i][5] =
+av_clip_uintp2(base_qi +