Discussion:
New VDA hwaccel
(too old to reply)
Luca Barbato
2014-03-24 02:01:44 UTC
Permalink
I managed to get the new vda hwaccel working on a third party player correctly
and works decently with avconv.

The changes by Anton made hwaccel slightly nicer already (I'm calling it
hwaccel 1.2 since it is a good step further in the right direction), next I'd
hide the custom alloc/setup/free function in avcodec_open2/close
and make available the frame rendering that happens in avcodec_${hwaccel}
in decode_video2.

Optional parameters and activation would be fed through the avdictionary.

That way hwaccel would be seamless (but not as fast as possible) for everybody
and still letting thinkering (and top speed) for those that care.

(the usual suspects should get working patches soon)
Luca Barbato
2014-03-24 02:01:45 UTC
Permalink
From: Anton Khirnov <***@khirnov.net>

It will be useful in the following commits.
---
libavcodec/8bps.c | 2 +-
libavcodec/h263dec.c | 2 +-
libavcodec/h264_slice.c | 10 +++++-----
libavcodec/internal.h | 7 +++++++
libavcodec/mpeg12dec.c | 4 ++--
libavcodec/utils.c | 5 +++++
libavcodec/vc1dec.c | 2 +-
7 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/libavcodec/8bps.c b/libavcodec/8bps.c
index cfeb486..3fd15e0 100644
--- a/libavcodec/8bps.c
+++ b/libavcodec/8bps.c
@@ -159,7 +159,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
c->planemap[0] = 0; // 1st plane is palette indexes
break;
case 24:
- avctx->pix_fmt = avctx->get_format(avctx, pixfmt_rgb24);
+ avctx->pix_fmt = ff_get_format(avctx, pixfmt_rgb24);
c->planes = 3;
c->planemap[0] = 2; // 1st plane is red
c->planemap[1] = 1; // 2nd plane is green
diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c
index 6c2f322..602dafa 100644
--- a/libavcodec/h263dec.c
+++ b/libavcodec/h263dec.c
@@ -57,7 +57,7 @@ av_cold int ff_h263_decode_init(AVCodecContext *avctx)
if (avctx->codec->id == AV_CODEC_ID_MSS2)
avctx->pix_fmt = AV_PIX_FMT_YUV420P;
else
- avctx->pix_fmt = avctx->get_format(avctx, avctx->codec->pix_fmts);
+ avctx->pix_fmt = ff_get_format(avctx, avctx->codec->pix_fmts);
s->unrestricted_mv = 1;

/* select sub codec */
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 897c8eb..683dead 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -1011,11 +1011,11 @@ static enum AVPixelFormat get_pixel_format(H264Context *h)
return h->avctx->color_range == AVCOL_RANGE_JPEG ? AV_PIX_FMT_YUVJ422P
: AV_PIX_FMT_YUV422P;
} else {
- return h->avctx->get_format(h->avctx, h->avctx->codec->pix_fmts ?
- h->avctx->codec->pix_fmts :
- h->avctx->color_range == AVCOL_RANGE_JPEG ?
- h264_hwaccel_pixfmt_list_jpeg_420 :
- h264_hwaccel_pixfmt_list_420);
+ return ff_get_format(h->avctx, h->avctx->codec->pix_fmts ?
+ h->avctx->codec->pix_fmts :
+ h->avctx->color_range == AVCOL_RANGE_JPEG ?
+ h264_hwaccel_pixfmt_list_jpeg_420 :
+ h264_hwaccel_pixfmt_list_420);
}
break;
default:
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index 9f7213c..eb91b6a 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -186,4 +186,11 @@ int ff_set_dimensions(AVCodecContext *s, int width, int height);
int ff_side_data_update_matrix_encoding(AVFrame *frame,
enum AVMatrixEncoding matrix_encoding);

+/**
+ * Select the (possibly hardware accelerated) pixel format.
+ * This is a wrapper around AVCodecContext.get_format() and should be used
+ * instead of calling get_format() directly.
+ */
+int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt);
+
#endif /* AVCODEC_INTERNAL_H */
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 2095eea..b8e7d36 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1188,12 +1188,12 @@ static enum AVPixelFormat mpeg_get_pixelformat(AVCodecContext *avctx)
#if FF_API_XVMC
FF_DISABLE_DEPRECATION_WARNINGS
if (avctx->xvmc_acceleration)
- return avctx->get_format(avctx, pixfmt_xvmc_mpg2_420);
+ return ff_get_format(avctx, pixfmt_xvmc_mpg2_420);
FF_ENABLE_DEPRECATION_WARNINGS
#endif /* FF_API_XVMC */

if (s->chroma_format < 2)
- return avctx->get_format(avctx, mpeg12_hwaccel_pixfmt_list_420);
+ return ff_get_format(avctx, mpeg12_hwaccel_pixfmt_list_420);
else if (s->chroma_format == 2)
return AV_PIX_FMT_YUV422P;
else
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index c88b346..2b3c00a 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -836,6 +836,11 @@ enum AVPixelFormat avcodec_default_get_format(struct AVCodecContext *s, const en
return fmt[0];
}

+int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
+{
+ return avctx->get_format(avctx, fmt);
+}
+
#if FF_API_AVFRAME_LAVC
void avcodec_get_frame_defaults(AVFrame *frame)
{
diff --git a/libavcodec/vc1dec.c b/libavcodec/vc1dec.c
index f8340ee..caf33fb 100644
--- a/libavcodec/vc1dec.c
+++ b/libavcodec/vc1dec.c
@@ -5594,7 +5594,7 @@ static av_cold int vc1_decode_init(AVCodecContext *avctx)
if (!avctx->extradata_size || !avctx->extradata)
return -1;
if (!(avctx->flags & CODEC_FLAG_GRAY))
- avctx->pix_fmt = avctx->get_format(avctx, avctx->codec->pix_fmts);
+ avctx->pix_fmt = ff_get_format(avctx, avctx->codec->pix_fmts);
else
avctx->pix_fmt = AV_PIX_FMT_GRAY8;
avctx->hwaccel = ff_find_hwaccel(avctx);
--
1.8.5.2 (Apple Git-48)
Vittorio Giovara
2014-03-24 02:25:46 UTC
Permalink
On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
> From: Anton Khirnov <***@khirnov.net>
>
> It will be useful in the following commits.
> ---
> libavcodec/8bps.c | 2 +-
> libavcodec/h263dec.c | 2 +-
> libavcodec/h264_slice.c | 10 +++++-----
> libavcodec/internal.h | 7 +++++++
> libavcodec/mpeg12dec.c | 4 ++--
> libavcodec/utils.c | 5 +++++
> libavcodec/vc1dec.c | 2 +-
> 7 files changed, 22 insertions(+), 10 deletions(-)
>

OK I think.
--
Vittorio
Luca Barbato
2014-03-24 02:01:46 UTC
Permalink
From: Anton Khirnov <***@khirnov.net>

This way each decoder does not have to do the same thing manually.
---
libavcodec/h263dec.c | 1 -
libavcodec/h264_slice.c | 2 --
libavcodec/internal.h | 9 ---------
libavcodec/mpeg12dec.c | 2 --
libavcodec/utils.c | 48 +++++++++++++++++++++++++++++++++---------------
libavcodec/vc1dec.c | 1 -
6 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c
index 602dafa..ddc3b01 100644
--- a/libavcodec/h263dec.c
+++ b/libavcodec/h263dec.c
@@ -108,7 +108,6 @@ av_cold int ff_h263_decode_init(AVCodecContext *avctx)
return AVERROR(ENOSYS);
}
s->codec_id = avctx->codec->id;
- avctx->hwaccel = ff_find_hwaccel(avctx);

/* for h263, we allocate the images after having read the header */
if (avctx->codec->id != AV_CODEC_ID_H263 &&
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 683dead..d6a25c4 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -1081,8 +1081,6 @@ static int h264_slice_header_init(H264Context *h, int reinit)
h->sps.num_units_in_tick, den, 1 << 30);
}

- h->avctx->hwaccel = ff_find_hwaccel(h->avctx);
-
if (reinit)
ff_h264_free_tables(h, 0);
h->first_field = 0;
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index eb91b6a..64765a2 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -103,15 +103,6 @@ struct AVCodecDefault {
};

/**
- * Return the hardware accelerated codec for codec codec_id and
- * pixel format pix_fmt.
- *
- * @param avctx The codec context containing the codec_id and pixel format.
- * @return the hardware accelerated codec, or NULL if none was found.
- */
-AVHWAccel *ff_find_hwaccel(AVCodecContext *avctx);
-
-/**
* Return the index into tab at which {a,b} match elements {[0],[1]} of tab.
* If there is no such matching pair then size is returned.
*/
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index b8e7d36..c3f06dc 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1295,7 +1295,6 @@ static int mpeg_decode_postinit(AVCodecContext *avctx)
} // MPEG-2

avctx->pix_fmt = mpeg_get_pixelformat(avctx);
- avctx->hwaccel = ff_find_hwaccel(avctx);
// until then pix_fmt may be changed right after codec init
#if FF_API_XVMC
if ((avctx->pix_fmt == AV_PIX_FMT_XVMC_MPEG2_IDCT ||
@@ -2126,7 +2125,6 @@ static int vcr2_init_sequence(AVCodecContext *avctx)
s->low_delay = 1;

avctx->pix_fmt = mpeg_get_pixelformat(avctx);
- avctx->hwaccel = ff_find_hwaccel(avctx);

#if FF_API_XVMC
if ((avctx->pix_fmt == AV_PIX_FMT_XVMC_MPEG2_IDCT || avctx->hwaccel) &&
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 2b3c00a..e52d915 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -836,9 +836,41 @@ enum AVPixelFormat avcodec_default_get_format(struct AVCodecContext *s, const en
return fmt[0];
}

+static AVHWAccel *find_hwaccel(enum AVCodecID codec_id,
+ enum AVPixelFormat pix_fmt)
+{
+ AVHWAccel *hwaccel = NULL;
+
+ while ((hwaccel = av_hwaccel_next(hwaccel)))
+ if (hwaccel->id == codec_id
+ && hwaccel->pix_fmt == pix_fmt)
+ return hwaccel;
+ return NULL;
+}
+
+
int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
{
- return avctx->get_format(avctx, fmt);
+ const AVPixFmtDescriptor *desc;
+ enum AVPixelFormat ret = avctx->get_format(avctx, fmt);
+
+ desc = av_pix_fmt_desc_get(ret);
+ if (!desc)
+ return AV_PIX_FMT_NONE;
+
+ if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
+ avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
+ if (!avctx->hwaccel) {
+ av_log(avctx, AV_LOG_ERROR,
+ "Could not find an AVHWAccel for the pixel format: %s",
+ desc->name);
+ return AV_PIX_FMT_NONE;
+ }
+ } else {
+ avctx->hwaccel = NULL;
+ }
+
+ return ret;
}

#if FF_API_AVFRAME_LAVC
@@ -2181,20 +2213,6 @@ AVHWAccel *av_hwaccel_next(AVHWAccel *hwaccel)
return hwaccel ? hwaccel->next : first_hwaccel;
}

-AVHWAccel *ff_find_hwaccel(AVCodecContext *avctx)
-{
- enum AVCodecID codec_id = avctx->codec->id;
- enum AVPixelFormat pix_fmt = avctx->pix_fmt;
-
- AVHWAccel *hwaccel = NULL;
-
- while ((hwaccel = av_hwaccel_next(hwaccel)))
- if (hwaccel->id == codec_id
- && hwaccel->pix_fmt == pix_fmt)
- return hwaccel;
- return NULL;
-}
-
int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
{
if (lockmgr_cb) {
diff --git a/libavcodec/vc1dec.c b/libavcodec/vc1dec.c
index caf33fb..b04e22d 100644
--- a/libavcodec/vc1dec.c
+++ b/libavcodec/vc1dec.c
@@ -5597,7 +5597,6 @@ static av_cold int vc1_decode_init(AVCodecContext *avctx)
avctx->pix_fmt = ff_get_format(avctx, avctx->codec->pix_fmts);
else
avctx->pix_fmt = AV_PIX_FMT_GRAY8;
- avctx->hwaccel = ff_find_hwaccel(avctx);
v->s.avctx = avctx;

if (ff_vc1_init_common(v) < 0)
--
1.8.5.2 (Apple Git-48)
Vittorio Giovara
2014-03-24 02:24:16 UTC
Permalink
On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
> From: Anton Khirnov <***@khirnov.net>
>
> This way each decoder does not have to do the same thing manually.
> ---
> libavcodec/h263dec.c | 1 -
> libavcodec/h264_slice.c | 2 --
> libavcodec/internal.h | 9 ---------
> libavcodec/mpeg12dec.c | 2 --
> libavcodec/utils.c | 48 +++++++++++++++++++++++++++++++++---------------
> libavcodec/vc1dec.c | 1 -
> 6 files changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c
> index 602dafa..ddc3b01 100644
> --- a/libavcodec/h263dec.c
> +++ b/libavcodec/h263dec.c
> @@ -108,7 +108,6 @@ av_cold int ff_h263_decode_init(AVCodecContext *avctx)
> return AVERROR(ENOSYS);
> }
> s->codec_id = avctx->codec->id;
> - avctx->hwaccel = ff_find_hwaccel(avctx);
>
> /* for h263, we allocate the images after having read the header */
> if (avctx->codec->id != AV_CODEC_ID_H263 &&
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 683dead..d6a25c4 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -1081,8 +1081,6 @@ static int h264_slice_header_init(H264Context *h, int reinit)
> h->sps.num_units_in_tick, den, 1 << 30);
> }
>
> - h->avctx->hwaccel = ff_find_hwaccel(h->avctx);
> -
> if (reinit)
> ff_h264_free_tables(h, 0);
> h->first_field = 0;
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index eb91b6a..64765a2 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -103,15 +103,6 @@ struct AVCodecDefault {
> };
>
> /**
> - * Return the hardware accelerated codec for codec codec_id and
> - * pixel format pix_fmt.
> - *
> - * @param avctx The codec context containing the codec_id and pixel format.
> - * @return the hardware accelerated codec, or NULL if none was found.
> - */
> -AVHWAccel *ff_find_hwaccel(AVCodecContext *avctx);
> -
> -/**
> * Return the index into tab at which {a,b} match elements {[0],[1]} of tab.
> * If there is no such matching pair then size is returned.
> */
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index b8e7d36..c3f06dc 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -1295,7 +1295,6 @@ static int mpeg_decode_postinit(AVCodecContext *avctx)
> } // MPEG-2
>
> avctx->pix_fmt = mpeg_get_pixelformat(avctx);
> - avctx->hwaccel = ff_find_hwaccel(avctx);
> // until then pix_fmt may be changed right after codec init
> #if FF_API_XVMC
> if ((avctx->pix_fmt == AV_PIX_FMT_XVMC_MPEG2_IDCT ||
> @@ -2126,7 +2125,6 @@ static int vcr2_init_sequence(AVCodecContext *avctx)
> s->low_delay = 1;
>
> avctx->pix_fmt = mpeg_get_pixelformat(avctx);
> - avctx->hwaccel = ff_find_hwaccel(avctx);
>
> #if FF_API_XVMC
> if ((avctx->pix_fmt == AV_PIX_FMT_XVMC_MPEG2_IDCT || avctx->hwaccel) &&
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 2b3c00a..e52d915 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -836,9 +836,41 @@ enum AVPixelFormat avcodec_default_get_format(struct AVCodecContext *s, const en
> return fmt[0];
> }
>
> +static AVHWAccel *find_hwaccel(enum AVCodecID codec_id,
> + enum AVPixelFormat pix_fmt)
> +{
> + AVHWAccel *hwaccel = NULL;
> +
> + while ((hwaccel = av_hwaccel_next(hwaccel)))
> + if (hwaccel->id == codec_id
> + && hwaccel->pix_fmt == pix_fmt)
> + return hwaccel;
> + return NULL;
> +}
> +
> +

nit: extra line added

> int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
> {
> - return avctx->get_format(avctx, fmt);
> + const AVPixFmtDescriptor *desc;
> + enum AVPixelFormat ret = avctx->get_format(avctx, fmt);
> +
> + desc = av_pix_fmt_desc_get(ret);
> + if (!desc)
> + return AV_PIX_FMT_NONE;
> +
> + if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
> + avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
> + if (!avctx->hwaccel) {
> + av_log(avctx, AV_LOG_ERROR,
> + "Could not find an AVHWAccel for the pixel format: %s",
> + desc->name);
> + return AV_PIX_FMT_NONE;
> + }
> + } else {
> + avctx->hwaccel = NULL;
> + }
> +
> + return ret;
> }
>
> #if FF_API_AVFRAME_LAVC
> @@ -2181,20 +2213,6 @@ AVHWAccel *av_hwaccel_next(AVHWAccel *hwaccel)
> return hwaccel ? hwaccel->next : first_hwaccel;
> }
>
> -AVHWAccel *ff_find_hwaccel(AVCodecContext *avctx)
> -{
> - enum AVCodecID codec_id = avctx->codec->id;
> - enum AVPixelFormat pix_fmt = avctx->pix_fmt;
> -
> - AVHWAccel *hwaccel = NULL;
> -
> - while ((hwaccel = av_hwaccel_next(hwaccel)))
> - if (hwaccel->id == codec_id
> - && hwaccel->pix_fmt == pix_fmt)
> - return hwaccel;
> - return NULL;
> -}
> -
> int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
> {
> if (lockmgr_cb) {
> diff --git a/libavcodec/vc1dec.c b/libavcodec/vc1dec.c
> index caf33fb..b04e22d 100644
> --- a/libavcodec/vc1dec.c
> +++ b/libavcodec/vc1dec.c
> @@ -5597,7 +5597,6 @@ static av_cold int vc1_decode_init(AVCodecContext *avctx)
> avctx->pix_fmt = ff_get_format(avctx, avctx->codec->pix_fmts);
> else
> avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> - avctx->hwaccel = ff_find_hwaccel(avctx);
> v->s.avctx = avctx;
>
> if (ff_vc1_init_common(v) < 0)
> --
Probably ok
--
Vittorio
Rémi Denis-Courmont
2014-03-24 07:46:01 UTC
Permalink
On Mon, 24 Mar 2014 03:01:46 +0100, Luca Barbato <***@gentoo.org>
wrote:
> From: Anton Khirnov <***@khirnov.net>
>
> This way each decoder does not have to do the same thing manually.
> ---
> libavcodec/h263dec.c | 1 -
> libavcodec/h264_slice.c | 2 --
> libavcodec/internal.h | 9 ---------
> libavcodec/mpeg12dec.c | 2 --
> libavcodec/utils.c | 48
> +++++++++++++++++++++++++++++++++---------------
> libavcodec/vc1dec.c | 1 -
> 6 files changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 2b3c00a..e52d915 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -836,9 +836,41 @@ enum AVPixelFormat
avcodec_default_get_format(struct
> AVCodecContext *s, const en
> return fmt[0];
> }
>
> +static AVHWAccel *find_hwaccel(enum AVCodecID codec_id,
> + enum AVPixelFormat pix_fmt)
> +{
> + AVHWAccel *hwaccel = NULL;
> +
> + while ((hwaccel = av_hwaccel_next(hwaccel)))
> + if (hwaccel->id == codec_id
> + && hwaccel->pix_fmt == pix_fmt)
> + return hwaccel;
> + return NULL;
> +}
> +
> +
> int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
> {
> - return avctx->get_format(avctx, fmt);
> + const AVPixFmtDescriptor *desc;
> + enum AVPixelFormat ret = avctx->get_format(avctx, fmt);
> +
> + desc = av_pix_fmt_desc_get(ret);
> + if (!desc)
> + return AV_PIX_FMT_NONE;
> +
> + if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
> + avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
> + if (!avctx->hwaccel) {
> + av_log(avctx, AV_LOG_ERROR,
> + "Could not find an AVHWAccel for the pixel format:
%s",
> + desc->name);
> + return AV_PIX_FMT_NONE;

Is this recoverable in any reasonably way?

I can see three ways this situation happens:

1) libavcodec advertised a pxiel format in get_format() that it does not
actually support. That would be a bug in libavcodec (this bug actually does
exist if you disable some hwaccel manually in ./configure).

2) The application returned a pixel format that was not advertised. I
think that is a non-recoverable application bug.

3) Initialization of the hwaccel back-end unexpectedly fail. I am not sure
if this should be allowed at this step. But if it should, I think the
correct solution is to call get_format again, *without* the failed pixel
format in the list. That way the application has a chance to recover by
falling back to some other format.

> + }
> + } else {
> + avctx->hwaccel = NULL;
> + }
> +
> + return ret;
> }
>
> #if FF_API_AVFRAME_LAVC

--
Rémi Denis-Courmont
Sent from my collocated server
Luca Barbato
2014-03-24 02:01:47 UTC
Permalink
From: Anton Khirnov <***@khirnov.net>

---
libavcodec/avcodec.h | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 7beb277..fd5f300 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2888,6 +2888,13 @@ typedef struct AVHWAccel {
*/
int capabilities;

+ /*****************************************************************
+ * No fields below this line are part of the public API. They
+ * may not be used outside of libavcodec and can be changed and
+ * removed at will.
+ * New public fields should be added right above.
+ *****************************************************************
+ */
struct AVHWAccel *next;

/**
--
1.8.5.2 (Apple Git-48)
Vittorio Giovara
2014-03-24 02:22:24 UTC
Permalink
On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
> From: Anton Khirnov <***@khirnov.net>
>
> ---
> libavcodec/avcodec.h | 7 +++++++
> 1 file changed, 7 insertions(+)
>
OK
--
Vittorio
Luca Barbato
2014-03-24 02:01:48 UTC
Permalink
From: Anton Khirnov <***@khirnov.net>

This describes more accurately what this field is for.
---
libavcodec/avcodec.h | 4 ++--
libavcodec/dxva2_h264.c | 2 +-
libavcodec/dxva2_mpeg2.c | 2 +-
libavcodec/dxva2_vc1.c | 4 ++--
libavcodec/h264_slice.c | 4 ++--
libavcodec/mpegvideo.c | 4 ++--
libavcodec/vdpau_h264.c | 2 +-
libavcodec/vdpau_mpeg12.c | 4 ++--
libavcodec/vdpau_mpeg4.c | 4 ++--
libavcodec/vdpau_vc1.c | 4 ++--
10 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index fd5f300..264305d 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2938,13 +2938,13 @@ typedef struct AVHWAccel {
int (*end_frame)(AVCodecContext *avctx);

/**
- * Size of HW accelerator private data.
+ * Size of per-frame HW accelerator private data.
*
* Private data is allocated with av_mallocz() before
* AVCodecContext.get_buffer() and deallocated after
* AVCodecContext.release_buffer().
*/
- int priv_data_size;
+ int frame_priv_data_size;
} AVHWAccel;

/**
diff --git a/libavcodec/dxva2_h264.c b/libavcodec/dxva2_h264.c
index 663eb7d..33db673 100644
--- a/libavcodec/dxva2_h264.c
+++ b/libavcodec/dxva2_h264.c
@@ -449,5 +449,5 @@ AVHWAccel ff_h264_dxva2_hwaccel = {
.start_frame = dxva2_h264_start_frame,
.decode_slice = dxva2_h264_decode_slice,
.end_frame = dxva2_h264_end_frame,
- .priv_data_size = sizeof(struct dxva2_picture_context),
+ .frame_priv_data_size = sizeof(struct dxva2_picture_context),
};
diff --git a/libavcodec/dxva2_mpeg2.c b/libavcodec/dxva2_mpeg2.c
index b6c2361..59b55e5 100644
--- a/libavcodec/dxva2_mpeg2.c
+++ b/libavcodec/dxva2_mpeg2.c
@@ -276,5 +276,5 @@ AVHWAccel ff_mpeg2_dxva2_hwaccel = {
.start_frame = dxva2_mpeg2_start_frame,
.decode_slice = dxva2_mpeg2_decode_slice,
.end_frame = dxva2_mpeg2_end_frame,
- .priv_data_size = sizeof(struct dxva2_picture_context),
+ .frame_priv_data_size = sizeof(struct dxva2_picture_context),
};
diff --git a/libavcodec/dxva2_vc1.c b/libavcodec/dxva2_vc1.c
index b2614dd..e5f893f 100644
--- a/libavcodec/dxva2_vc1.c
+++ b/libavcodec/dxva2_vc1.c
@@ -279,7 +279,7 @@ AVHWAccel ff_wmv3_dxva2_hwaccel = {
.start_frame = dxva2_vc1_start_frame,
.decode_slice = dxva2_vc1_decode_slice,
.end_frame = dxva2_vc1_end_frame,
- .priv_data_size = sizeof(struct dxva2_picture_context),
+ .frame_priv_data_size = sizeof(struct dxva2_picture_context),
};
#endif

@@ -291,5 +291,5 @@ AVHWAccel ff_vc1_dxva2_hwaccel = {
.start_frame = dxva2_vc1_start_frame,
.decode_slice = dxva2_vc1_decode_slice,
.end_frame = dxva2_vc1_end_frame,
- .priv_data_size = sizeof(struct dxva2_picture_context),
+ .frame_priv_data_size = sizeof(struct dxva2_picture_context),
};
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index d6a25c4..73c0740 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -258,8 +258,8 @@ static int alloc_picture(H264Context *h, H264Picture *pic)
if (h->avctx->hwaccel) {
const AVHWAccel *hwaccel = h->avctx->hwaccel;
av_assert0(!pic->hwaccel_picture_private);
- if (hwaccel->priv_data_size) {
- pic->hwaccel_priv_buf = av_buffer_allocz(hwaccel->priv_data_size);
+ if (hwaccel->frame_priv_data_size) {
+ pic->hwaccel_priv_buf = av_buffer_allocz(hwaccel->frame_priv_data_size);
if (!pic->hwaccel_priv_buf)
return AVERROR(ENOMEM);
pic->hwaccel_picture_private = pic->hwaccel_priv_buf->data;
diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
index 0fc77e8..79f1b4c 100644
--- a/libavcodec/mpegvideo.c
+++ b/libavcodec/mpegvideo.c
@@ -460,8 +460,8 @@ static int alloc_frame_buffer(MpegEncContext *s, Picture *pic)

if (s->avctx->hwaccel) {
assert(!pic->hwaccel_picture_private);
- if (s->avctx->hwaccel->priv_data_size) {
- pic->hwaccel_priv_buf = av_buffer_allocz(s->avctx->hwaccel->priv_data_size);
+ if (s->avctx->hwaccel->frame_priv_data_size) {
+ pic->hwaccel_priv_buf = av_buffer_allocz(s->avctx->hwaccel->frame_priv_data_size);
if (!pic->hwaccel_priv_buf) {
av_log(s->avctx, AV_LOG_ERROR, "alloc_frame_buffer() failed (hwaccel private data allocation)\n");
return -1;
diff --git a/libavcodec/vdpau_h264.c b/libavcodec/vdpau_h264.c
index 7aa17ef..32e9c28 100644
--- a/libavcodec/vdpau_h264.c
+++ b/libavcodec/vdpau_h264.c
@@ -212,5 +212,5 @@ AVHWAccel ff_h264_vdpau_hwaccel = {
.start_frame = vdpau_h264_start_frame,
.end_frame = vdpau_h264_end_frame,
.decode_slice = vdpau_h264_decode_slice,
- .priv_data_size = sizeof(struct vdpau_picture_context),
+ .frame_priv_data_size = sizeof(struct vdpau_picture_context),
};
diff --git a/libavcodec/vdpau_mpeg12.c b/libavcodec/vdpau_mpeg12.c
index 0f92cef..18ff664 100644
--- a/libavcodec/vdpau_mpeg12.c
+++ b/libavcodec/vdpau_mpeg12.c
@@ -103,7 +103,7 @@ AVHWAccel ff_mpeg1_vdpau_hwaccel = {
.start_frame = vdpau_mpeg_start_frame,
.end_frame = ff_vdpau_mpeg_end_frame,
.decode_slice = vdpau_mpeg_decode_slice,
- .priv_data_size = sizeof(struct vdpau_picture_context),
+ .frame_priv_data_size = sizeof(struct vdpau_picture_context),
};
#endif

@@ -116,6 +116,6 @@ AVHWAccel ff_mpeg2_vdpau_hwaccel = {
.start_frame = vdpau_mpeg_start_frame,
.end_frame = ff_vdpau_mpeg_end_frame,
.decode_slice = vdpau_mpeg_decode_slice,
- .priv_data_size = sizeof(struct vdpau_picture_context),
+ .frame_priv_data_size = sizeof(struct vdpau_picture_context),
};
#endif
diff --git a/libavcodec/vdpau_mpeg4.c b/libavcodec/vdpau_mpeg4.c
index 3c6c8b3..6d28e17 100644
--- a/libavcodec/vdpau_mpeg4.c
+++ b/libavcodec/vdpau_mpeg4.c
@@ -97,7 +97,7 @@ AVHWAccel ff_h263_vdpau_hwaccel = {
.start_frame = vdpau_mpeg4_start_frame,
.end_frame = ff_vdpau_mpeg_end_frame,
.decode_slice = vdpau_mpeg4_decode_slice,
- .priv_data_size = sizeof(struct vdpau_picture_context),
+ .frame_priv_data_size = sizeof(struct vdpau_picture_context),
};
#endif

@@ -110,6 +110,6 @@ AVHWAccel ff_mpeg4_vdpau_hwaccel = {
.start_frame = vdpau_mpeg4_start_frame,
.end_frame = ff_vdpau_mpeg_end_frame,
.decode_slice = vdpau_mpeg4_decode_slice,
- .priv_data_size = sizeof(struct vdpau_picture_context),
+ .frame_priv_data_size = sizeof(struct vdpau_picture_context),
};
#endif
diff --git a/libavcodec/vdpau_vc1.c b/libavcodec/vdpau_vc1.c
index 392c511..5c58760 100644
--- a/libavcodec/vdpau_vc1.c
+++ b/libavcodec/vdpau_vc1.c
@@ -118,7 +118,7 @@ AVHWAccel ff_wmv3_vdpau_hwaccel = {
.start_frame = vdpau_vc1_start_frame,
.end_frame = ff_vdpau_mpeg_end_frame,
.decode_slice = vdpau_vc1_decode_slice,
- .priv_data_size = sizeof(struct vdpau_picture_context),
+ .frame_priv_data_size = sizeof(struct vdpau_picture_context),
};
#endif

@@ -130,5 +130,5 @@ AVHWAccel ff_vc1_vdpau_hwaccel = {
.start_frame = vdpau_vc1_start_frame,
.end_frame = ff_vdpau_mpeg_end_frame,
.decode_slice = vdpau_vc1_decode_slice,
- .priv_data_size = sizeof(struct vdpau_picture_context),
+ .frame_priv_data_size = sizeof(struct vdpau_picture_context),
};
--
1.8.5.2 (Apple Git-48)
Vittorio Giovara
2014-03-24 02:21:43 UTC
Permalink
On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
> From: Anton Khirnov <***@khirnov.net>
>
> This describes more accurately what this field is for.
> ---
> libavcodec/avcodec.h | 4 ++--
> libavcodec/dxva2_h264.c | 2 +-
> libavcodec/dxva2_mpeg2.c | 2 +-
> libavcodec/dxva2_vc1.c | 4 ++--
> libavcodec/h264_slice.c | 4 ++--
> libavcodec/mpegvideo.c | 4 ++--
> libavcodec/vdpau_h264.c | 2 +-
> libavcodec/vdpau_mpeg12.c | 4 ++--
> libavcodec/vdpau_mpeg4.c | 4 ++--
> libavcodec/vdpau_vc1.c | 4 ++--
> 10 files changed, 17 insertions(+), 17 deletions(-)
>

Probably OK but I think you should bump avcodec's minor.
--
Vittorio
Luca Barbato
2014-03-24 02:01:49 UTC
Permalink
---
libavcodec/pthread_frame.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
index 1af8ff5..558a518 100644
--- a/libavcodec/pthread_frame.c
+++ b/libavcodec/pthread_frame.c
@@ -206,6 +206,7 @@ static int update_context_from_thread(AVCodecContext *dst, AVCodecContext *src,

dst->hwaccel = src->hwaccel;
dst->hwaccel_context = src->hwaccel_context;
+ dst->internal->hwaccel_priv_data = src->internal->hwaccel_priv_data;
}

if (for_user) {
--
1.8.5.2 (Apple Git-48)
Vittorio Giovara
2014-03-24 02:20:59 UTC
Permalink
On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
> ---
> libavcodec/pthread_frame.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 1af8ff5..558a518 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -206,6 +206,7 @@ static int update_context_from_thread(AVCodecContext *dst, AVCodecContext *src,
>
> dst->hwaccel = src->hwaccel;
> dst->hwaccel_context = src->hwaccel_context;
> + dst->internal->hwaccel_priv_data = src->internal->hwaccel_priv_data;
> }
>
> if (for_user) {

ok
Vittorio
Luca Barbato
2014-03-24 02:01:50 UTC
Permalink
From: Anton Khirnov <***@khirnov.net>

---
libavcodec/avcodec.h | 23 +++++++++++++++++++++++
libavcodec/internal.h | 5 +++++
libavcodec/utils.c | 34 ++++++++++++++++++++++++++++++----
3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 264305d..a1d0ae5 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2945,6 +2945,29 @@ typedef struct AVHWAccel {
* AVCodecContext.release_buffer().
*/
int frame_priv_data_size;
+
+ /**
+ * Initialize the hwaccel private data.
+ *
+ * This will be called from ff_get_format(), after hwaccel and
+ * hwaccel_context are set and the hwaccel private data in AVCodecInternal
+ * is allocated.
+ */
+ int (*init)(AVCodecContext *avctx);
+
+ /**
+ * Uninitialize the hwaccel private data.
+ *
+ * This will be called from get_format() or avcodec_close(), after hwaccel
+ * and hwaccel_context are already uninitialized.
+ */
+ int (*uninit)(AVCodecContext *avctx);
+
+ /**
+ * Size of the private data to allocate in
+ * AVCodecInternal.hwaccel_priv_data.
+ */
+ int priv_data_size;
} AVHWAccel;

/**
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index 64765a2..17de2d5 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -95,6 +95,11 @@ typedef struct AVCodecInternal {
* packet into every function.
*/
AVPacket *pkt;
+
+ /**
+ * hwaccel-specific private data
+ */
+ void *hwaccel_priv_data;
} AVCodecInternal;

struct AVCodecDefault {
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index e52d915..5ca7534 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -858,16 +858,37 @@ int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
if (!desc)
return AV_PIX_FMT_NONE;

+ if (avctx->hwaccel && avctx->hwaccel->uninit)
+ avctx->hwaccel->uninit(avctx);
+ av_freep(&avctx->internal->hwaccel_priv_data);
+ avctx->hwaccel = NULL;
+
if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
- avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
- if (!avctx->hwaccel) {
+ AVHWAccel *hwaccel;
+ int err;
+
+ hwaccel = find_hwaccel(avctx->codec_id, ret);
+ if (!hwaccel) {
av_log(avctx, AV_LOG_ERROR,
"Could not find an AVHWAccel for the pixel format: %s",
desc->name);
return AV_PIX_FMT_NONE;
}
- } else {
- avctx->hwaccel = NULL;
+
+ if (hwaccel->priv_data_size) {
+ avctx->internal->hwaccel_priv_data = av_mallocz(hwaccel->priv_data_size);
+ if (!avctx->internal->hwaccel_priv_data)
+ return AV_PIX_FMT_NONE;
+ }
+
+ if (hwaccel->init) {
+ err = hwaccel->init(avctx);
+ if (err < 0) {
+ av_freep(&avctx->internal->hwaccel_priv_data);
+ return AV_PIX_FMT_NONE;
+ }
+ }
+ avctx->hwaccel = hwaccel;
}

return ret;
@@ -1653,6 +1674,11 @@ av_cold int avcodec_close(AVCodecContext *avctx)
for (i = 0; i < FF_ARRAY_ELEMS(pool->pools); i++)
av_buffer_pool_uninit(&pool->pools[i]);
av_freep(&avctx->internal->pool);
+
+ if (avctx->hwaccel && avctx->hwaccel->uninit)
+ avctx->hwaccel->uninit(avctx);
+ av_freep(&avctx->internal->hwaccel_priv_data);
+
av_freep(&avctx->internal);
}

--
1.8.5.2 (Apple Git-48)
Vittorio Giovara
2014-03-24 02:28:01 UTC
Permalink
On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
> From: Anton Khirnov <***@khirnov.net>
>
> ---
> libavcodec/avcodec.h | 23 +++++++++++++++++++++++
> libavcodec/internal.h | 5 +++++
> libavcodec/utils.c | 34 ++++++++++++++++++++++++++++++----
> 3 files changed, 58 insertions(+), 4 deletions(-)

I think there should be another minor bump here.

> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 264305d..a1d0ae5 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2945,6 +2945,29 @@ typedef struct AVHWAccel {
> * AVCodecContext.release_buffer().
> */
> int frame_priv_data_size;
> +
> + /**
> + * Initialize the hwaccel private data.
> + *
> + * This will be called from ff_get_format(), after hwaccel and
> + * hwaccel_context are set and the hwaccel private data in AVCodecInternal
> + * is allocated.
> + */
> + int (*init)(AVCodecContext *avctx);
> +
> + /**
> + * Uninitialize the hwaccel private data.
> + *
> + * This will be called from get_format() or avcodec_close(), after hwaccel
> + * and hwaccel_context are already uninitialized.
> + */
> + int (*uninit)(AVCodecContext *avctx);
> +
> + /**
> + * Size of the private data to allocate in
> + * AVCodecInternal.hwaccel_priv_data.
> + */
> + int priv_data_size;
> } AVHWAccel;
>
> /**
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index 64765a2..17de2d5 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -95,6 +95,11 @@ typedef struct AVCodecInternal {
> * packet into every function.
> */
> AVPacket *pkt;
> +
> + /**
> + * hwaccel-specific private data
> + */
> + void *hwaccel_priv_data;
> } AVCodecInternal;
>
> struct AVCodecDefault {
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index e52d915..5ca7534 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -858,16 +858,37 @@ int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
> if (!desc)
> return AV_PIX_FMT_NONE;
>
> + if (avctx->hwaccel && avctx->hwaccel->uninit)
> + avctx->hwaccel->uninit(avctx);
> + av_freep(&avctx->internal->hwaccel_priv_data);
> + avctx->hwaccel = NULL;
> +
> if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
> - avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
> - if (!avctx->hwaccel) {
> + AVHWAccel *hwaccel;
> + int err;
> +
> + hwaccel = find_hwaccel(avctx->codec_id, ret);
> + if (!hwaccel) {
> av_log(avctx, AV_LOG_ERROR,
> "Could not find an AVHWAccel for the pixel format: %s",
> desc->name);
> return AV_PIX_FMT_NONE;
> }
> - } else {
> - avctx->hwaccel = NULL;
> +
> + if (hwaccel->priv_data_size) {
> + avctx->internal->hwaccel_priv_data = av_mallocz(hwaccel->priv_data_size);
> + if (!avctx->internal->hwaccel_priv_data)
> + return AV_PIX_FMT_NONE;
> + }
> +
> + if (hwaccel->init) {
> + err = hwaccel->init(avctx);
> + if (err < 0) {
> + av_freep(&avctx->internal->hwaccel_priv_data);
> + return AV_PIX_FMT_NONE;
> + }
> + }
> + avctx->hwaccel = hwaccel;
> }
>
> return ret;
> @@ -1653,6 +1674,11 @@ av_cold int avcodec_close(AVCodecContext *avctx)
> for (i = 0; i < FF_ARRAY_ELEMS(pool->pools); i++)
> av_buffer_pool_uninit(&pool->pools[i]);
> av_freep(&avctx->internal->pool);
> +
> + if (avctx->hwaccel && avctx->hwaccel->uninit)
> + avctx->hwaccel->uninit(avctx);
> + av_freep(&avctx->internal->hwaccel_priv_data);
> +
> av_freep(&avctx->internal);
> }
>

Looks ok, but I wouldn't mind a second opinion.
Vittorio
Hendrik Leppkes
2014-03-24 09:07:48 UTC
Permalink
On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
> From: Anton Khirnov <***@khirnov.net>
>
> ---
> libavcodec/avcodec.h | 23 +++++++++++++++++++++++
> libavcodec/internal.h | 5 +++++
> libavcodec/utils.c | 34 ++++++++++++++++++++++++++++++----
> 3 files changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 264305d..a1d0ae5 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2945,6 +2945,29 @@ typedef struct AVHWAccel {
> * AVCodecContext.release_buffer().
> */
> int frame_priv_data_size;
> +
> + /**
> + * Initialize the hwaccel private data.
> + *
> + * This will be called from ff_get_format(), after hwaccel and
> + * hwaccel_context are set and the hwaccel private data in AVCodecInternal
> + * is allocated.
> + */
> + int (*init)(AVCodecContext *avctx);
> +
> + /**
> + * Uninitialize the hwaccel private data.
> + *
> + * This will be called from get_format() or avcodec_close(), after hwaccel
> + * and hwaccel_context are already uninitialized.
> + */
> + int (*uninit)(AVCodecContext *avctx);
> +
> + /**
> + * Size of the private data to allocate in
> + * AVCodecInternal.hwaccel_priv_data.
> + */
> + int priv_data_size;
> } AVHWAccel;
>
> /**
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index 64765a2..17de2d5 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -95,6 +95,11 @@ typedef struct AVCodecInternal {
> * packet into every function.
> */
> AVPacket *pkt;
> +
> + /**
> + * hwaccel-specific private data
> + */
> + void *hwaccel_priv_data;
> } AVCodecInternal;
>
> struct AVCodecDefault {
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index e52d915..5ca7534 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -858,16 +858,37 @@ int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
> if (!desc)
> return AV_PIX_FMT_NONE;
>
> + if (avctx->hwaccel && avctx->hwaccel->uninit)
> + avctx->hwaccel->uninit(avctx);
> + av_freep(&avctx->internal->hwaccel_priv_data);
> + avctx->hwaccel = NULL;
> +
> if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
> - avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
> - if (!avctx->hwaccel) {
> + AVHWAccel *hwaccel;
> + int err;
> +
> + hwaccel = find_hwaccel(avctx->codec_id, ret);
> + if (!hwaccel) {
> av_log(avctx, AV_LOG_ERROR,
> "Could not find an AVHWAccel for the pixel format: %s",
> desc->name);
> return AV_PIX_FMT_NONE;
> }
> - } else {
> - avctx->hwaccel = NULL;
> +
> + if (hwaccel->priv_data_size) {
> + avctx->internal->hwaccel_priv_data = av_mallocz(hwaccel->priv_data_size);
> + if (!avctx->internal->hwaccel_priv_data)
> + return AV_PIX_FMT_NONE;
> + }
> +
> + if (hwaccel->init) {
> + err = hwaccel->init(avctx);
> + if (err < 0) {
> + av_freep(&avctx->internal->hwaccel_priv_data);
> + return AV_PIX_FMT_NONE;
> + }
> + }
> + avctx->hwaccel = hwaccel;
> }
>

The seems to unconditionally call uninit/init everytime get_format is
called, is this really required? AFAIK its called more often then
really required.
Maybe check if you actually switch a hwaccel on or off, instead of
just re-initing it all the time?

- Hendrik
Luca Barbato
2014-03-24 09:12:31 UTC
Permalink
On 24/03/14 10:07, Hendrik Leppkes wrote:
> On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
>> From: Anton Khirnov <***@khirnov.net>
>>
>> ---
>> libavcodec/avcodec.h | 23 +++++++++++++++++++++++
>> libavcodec/internal.h | 5 +++++
>> libavcodec/utils.c | 34 ++++++++++++++++++++++++++++++----
>> 3 files changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index 264305d..a1d0ae5 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -2945,6 +2945,29 @@ typedef struct AVHWAccel {
>> * AVCodecContext.release_buffer().
>> */
>> int frame_priv_data_size;
>> +
>> + /**
>> + * Initialize the hwaccel private data.
>> + *
>> + * This will be called from ff_get_format(), after hwaccel and
>> + * hwaccel_context are set and the hwaccel private data in AVCodecInternal
>> + * is allocated.
>> + */
>> + int (*init)(AVCodecContext *avctx);
>> +
>> + /**
>> + * Uninitialize the hwaccel private data.
>> + *
>> + * This will be called from get_format() or avcodec_close(), after hwaccel
>> + * and hwaccel_context are already uninitialized.
>> + */
>> + int (*uninit)(AVCodecContext *avctx);
>> +
>> + /**
>> + * Size of the private data to allocate in
>> + * AVCodecInternal.hwaccel_priv_data.
>> + */
>> + int priv_data_size;
>> } AVHWAccel;
>>
>> /**
>> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
>> index 64765a2..17de2d5 100644
>> --- a/libavcodec/internal.h
>> +++ b/libavcodec/internal.h
>> @@ -95,6 +95,11 @@ typedef struct AVCodecInternal {
>> * packet into every function.
>> */
>> AVPacket *pkt;
>> +
>> + /**
>> + * hwaccel-specific private data
>> + */
>> + void *hwaccel_priv_data;
>> } AVCodecInternal;
>>
>> struct AVCodecDefault {
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index e52d915..5ca7534 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -858,16 +858,37 @@ int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
>> if (!desc)
>> return AV_PIX_FMT_NONE;
>>
>> + if (avctx->hwaccel && avctx->hwaccel->uninit)
>> + avctx->hwaccel->uninit(avctx);
>> + av_freep(&avctx->internal->hwaccel_priv_data);
>> + avctx->hwaccel = NULL;
>> +
>> if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
>> - avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
>> - if (!avctx->hwaccel) {
>> + AVHWAccel *hwaccel;
>> + int err;
>> +
>> + hwaccel = find_hwaccel(avctx->codec_id, ret);
>> + if (!hwaccel) {
>> av_log(avctx, AV_LOG_ERROR,
>> "Could not find an AVHWAccel for the pixel format: %s",
>> desc->name);
>> return AV_PIX_FMT_NONE;
>> }
>> - } else {
>> - avctx->hwaccel = NULL;
>> +
>> + if (hwaccel->priv_data_size) {
>> + avctx->internal->hwaccel_priv_data = av_mallocz(hwaccel->priv_data_size);
>> + if (!avctx->internal->hwaccel_priv_data)
>> + return AV_PIX_FMT_NONE;
>> + }
>> +
>> + if (hwaccel->init) {
>> + err = hwaccel->init(avctx);
>> + if (err < 0) {
>> + av_freep(&avctx->internal->hwaccel_priv_data);
>> + return AV_PIX_FMT_NONE;
>> + }
>> + }
>> + avctx->hwaccel = hwaccel;
>> }
>>
>
> The seems to unconditionally call uninit/init everytime get_format is
> called, is this really required? AFAIK its called more often then
> really required.
> Maybe check if you actually switch a hwaccel on or off, instead of
> just re-initing it all the time?

The get_format should be called only when there is a dimension/format
change.

lu
Hendrik Leppkes
2014-03-24 09:20:32 UTC
Permalink
On Mon, Mar 24, 2014 at 10:12 AM, Luca Barbato <***@gentoo.org> wrote:
> On 24/03/14 10:07, Hendrik Leppkes wrote:
>> On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
>>> From: Anton Khirnov <***@khirnov.net>
>>>
>>> ---
>>> libavcodec/avcodec.h | 23 +++++++++++++++++++++++
>>> libavcodec/internal.h | 5 +++++
>>> libavcodec/utils.c | 34 ++++++++++++++++++++++++++++++----
>>> 3 files changed, 58 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>> index 264305d..a1d0ae5 100644
>>> --- a/libavcodec/avcodec.h
>>> +++ b/libavcodec/avcodec.h
>>> @@ -2945,6 +2945,29 @@ typedef struct AVHWAccel {
>>> * AVCodecContext.release_buffer().
>>> */
>>> int frame_priv_data_size;
>>> +
>>> + /**
>>> + * Initialize the hwaccel private data.
>>> + *
>>> + * This will be called from ff_get_format(), after hwaccel and
>>> + * hwaccel_context are set and the hwaccel private data in AVCodecInternal
>>> + * is allocated.
>>> + */
>>> + int (*init)(AVCodecContext *avctx);
>>> +
>>> + /**
>>> + * Uninitialize the hwaccel private data.
>>> + *
>>> + * This will be called from get_format() or avcodec_close(), after hwaccel
>>> + * and hwaccel_context are already uninitialized.
>>> + */
>>> + int (*uninit)(AVCodecContext *avctx);
>>> +
>>> + /**
>>> + * Size of the private data to allocate in
>>> + * AVCodecInternal.hwaccel_priv_data.
>>> + */
>>> + int priv_data_size;
>>> } AVHWAccel;
>>>
>>> /**
>>> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
>>> index 64765a2..17de2d5 100644
>>> --- a/libavcodec/internal.h
>>> +++ b/libavcodec/internal.h
>>> @@ -95,6 +95,11 @@ typedef struct AVCodecInternal {
>>> * packet into every function.
>>> */
>>> AVPacket *pkt;
>>> +
>>> + /**
>>> + * hwaccel-specific private data
>>> + */
>>> + void *hwaccel_priv_data;
>>> } AVCodecInternal;
>>>
>>> struct AVCodecDefault {
>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>> index e52d915..5ca7534 100644
>>> --- a/libavcodec/utils.c
>>> +++ b/libavcodec/utils.c
>>> @@ -858,16 +858,37 @@ int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
>>> if (!desc)
>>> return AV_PIX_FMT_NONE;
>>>
>>> + if (avctx->hwaccel && avctx->hwaccel->uninit)
>>> + avctx->hwaccel->uninit(avctx);
>>> + av_freep(&avctx->internal->hwaccel_priv_data);
>>> + avctx->hwaccel = NULL;
>>> +
>>> if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
>>> - avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
>>> - if (!avctx->hwaccel) {
>>> + AVHWAccel *hwaccel;
>>> + int err;
>>> +
>>> + hwaccel = find_hwaccel(avctx->codec_id, ret);
>>> + if (!hwaccel) {
>>> av_log(avctx, AV_LOG_ERROR,
>>> "Could not find an AVHWAccel for the pixel format: %s",
>>> desc->name);
>>> return AV_PIX_FMT_NONE;
>>> }
>>> - } else {
>>> - avctx->hwaccel = NULL;
>>> +
>>> + if (hwaccel->priv_data_size) {
>>> + avctx->internal->hwaccel_priv_data = av_mallocz(hwaccel->priv_data_size);
>>> + if (!avctx->internal->hwaccel_priv_data)
>>> + return AV_PIX_FMT_NONE;
>>> + }
>>> +
>>> + if (hwaccel->init) {
>>> + err = hwaccel->init(avctx);
>>> + if (err < 0) {
>>> + av_freep(&avctx->internal->hwaccel_priv_data);
>>> + return AV_PIX_FMT_NONE;
>>> + }
>>> + }
>>> + avctx->hwaccel = hwaccel;
>>> }
>>>
>>
>> The seems to unconditionally call uninit/init everytime get_format is
>> called, is this really required? AFAIK its called more often then
>> really required.
>> Maybe check if you actually switch a hwaccel on or off, instead of
>> just re-initing it all the time?
>
> The get_format should be called only when there is a dimension/format
> change.
>

"should" is fine, but in my experience it isn't.
Luca Barbato
2014-03-24 09:34:02 UTC
Permalink
On 24/03/14 10:20, Hendrik Leppkes wrote:
> On Mon, Mar 24, 2014 at 10:12 AM, Luca Barbato <***@gentoo.org> wrote:
>> On 24/03/14 10:07, Hendrik Leppkes wrote:
>>> On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
>>>> From: Anton Khirnov <***@khirnov.net>
>>>>
>>>> ---
>>>> libavcodec/avcodec.h | 23 +++++++++++++++++++++++
>>>> libavcodec/internal.h | 5 +++++
>>>> libavcodec/utils.c | 34 ++++++++++++++++++++++++++++++----
>>>> 3 files changed, 58 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>> index 264305d..a1d0ae5 100644
>>>> --- a/libavcodec/avcodec.h
>>>> +++ b/libavcodec/avcodec.h
>>>> @@ -2945,6 +2945,29 @@ typedef struct AVHWAccel {
>>>> * AVCodecContext.release_buffer().
>>>> */
>>>> int frame_priv_data_size;
>>>> +
>>>> + /**
>>>> + * Initialize the hwaccel private data.
>>>> + *
>>>> + * This will be called from ff_get_format(), after hwaccel and
>>>> + * hwaccel_context are set and the hwaccel private data in AVCodecInternal
>>>> + * is allocated.
>>>> + */
>>>> + int (*init)(AVCodecContext *avctx);
>>>> +
>>>> + /**
>>>> + * Uninitialize the hwaccel private data.
>>>> + *
>>>> + * This will be called from get_format() or avcodec_close(), after hwaccel
>>>> + * and hwaccel_context are already uninitialized.
>>>> + */
>>>> + int (*uninit)(AVCodecContext *avctx);
>>>> +
>>>> + /**
>>>> + * Size of the private data to allocate in
>>>> + * AVCodecInternal.hwaccel_priv_data.
>>>> + */
>>>> + int priv_data_size;
>>>> } AVHWAccel;
>>>>
>>>> /**
>>>> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
>>>> index 64765a2..17de2d5 100644
>>>> --- a/libavcodec/internal.h
>>>> +++ b/libavcodec/internal.h
>>>> @@ -95,6 +95,11 @@ typedef struct AVCodecInternal {
>>>> * packet into every function.
>>>> */
>>>> AVPacket *pkt;
>>>> +
>>>> + /**
>>>> + * hwaccel-specific private data
>>>> + */
>>>> + void *hwaccel_priv_data;
>>>> } AVCodecInternal;
>>>>
>>>> struct AVCodecDefault {
>>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>>> index e52d915..5ca7534 100644
>>>> --- a/libavcodec/utils.c
>>>> +++ b/libavcodec/utils.c
>>>> @@ -858,16 +858,37 @@ int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
>>>> if (!desc)
>>>> return AV_PIX_FMT_NONE;
>>>>
>>>> + if (avctx->hwaccel && avctx->hwaccel->uninit)
>>>> + avctx->hwaccel->uninit(avctx);
>>>> + av_freep(&avctx->internal->hwaccel_priv_data);
>>>> + avctx->hwaccel = NULL;
>>>> +
>>>> if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
>>>> - avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
>>>> - if (!avctx->hwaccel) {
>>>> + AVHWAccel *hwaccel;
>>>> + int err;
>>>> +
>>>> + hwaccel = find_hwaccel(avctx->codec_id, ret);
>>>> + if (!hwaccel) {
>>>> av_log(avctx, AV_LOG_ERROR,
>>>> "Could not find an AVHWAccel for the pixel format: %s",
>>>> desc->name);
>>>> return AV_PIX_FMT_NONE;
>>>> }
>>>> - } else {
>>>> - avctx->hwaccel = NULL;
>>>> +
>>>> + if (hwaccel->priv_data_size) {
>>>> + avctx->internal->hwaccel_priv_data = av_mallocz(hwaccel->priv_data_size);
>>>> + if (!avctx->internal->hwaccel_priv_data)
>>>> + return AV_PIX_FMT_NONE;
>>>> + }
>>>> +
>>>> + if (hwaccel->init) {
>>>> + err = hwaccel->init(avctx);
>>>> + if (err < 0) {
>>>> + av_freep(&avctx->internal->hwaccel_priv_data);
>>>> + return AV_PIX_FMT_NONE;
>>>> + }
>>>> + }
>>>> + avctx->hwaccel = hwaccel;
>>>> }
>>>>
>>>
>>> The seems to unconditionally call uninit/init everytime get_format is
>>> called, is this really required? AFAIK its called more often then
>>> really required.
>>> Maybe check if you actually switch a hwaccel on or off, instead of
>>> just re-initing it all the time?
>>
>> The get_format should be called only when there is a dimension/format
>> change.
>>
>
> "should" is fine, but in my experience it isn't.

I'll look at the codepath to make sure =)
wm4
2014-03-24 19:26:41 UTC
Permalink
On Mon, 24 Mar 2014 10:34:02 +0100
Luca Barbato <***@gentoo.org> wrote:

> On 24/03/14 10:20, Hendrik Leppkes wrote:
> > On Mon, Mar 24, 2014 at 10:12 AM, Luca Barbato <***@gentoo.org> wrote:
> >> On 24/03/14 10:07, Hendrik Leppkes wrote:
> >>> On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
> >>>> From: Anton Khirnov <***@khirnov.net>
> >>>>
> >>>> ---
> >>>> libavcodec/avcodec.h | 23 +++++++++++++++++++++++
> >>>> libavcodec/internal.h | 5 +++++
> >>>> libavcodec/utils.c | 34 ++++++++++++++++++++++++++++++----
> >>>> 3 files changed, 58 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> >>>> index 264305d..a1d0ae5 100644
> >>>> --- a/libavcodec/avcodec.h
> >>>> +++ b/libavcodec/avcodec.h
> >>>> @@ -2945,6 +2945,29 @@ typedef struct AVHWAccel {
> >>>> * AVCodecContext.release_buffer().
> >>>> */
> >>>> int frame_priv_data_size;
> >>>> +
> >>>> + /**
> >>>> + * Initialize the hwaccel private data.
> >>>> + *
> >>>> + * This will be called from ff_get_format(), after hwaccel and
> >>>> + * hwaccel_context are set and the hwaccel private data in AVCodecInternal
> >>>> + * is allocated.
> >>>> + */
> >>>> + int (*init)(AVCodecContext *avctx);
> >>>> +
> >>>> + /**
> >>>> + * Uninitialize the hwaccel private data.
> >>>> + *
> >>>> + * This will be called from get_format() or avcodec_close(), after hwaccel
> >>>> + * and hwaccel_context are already uninitialized.
> >>>> + */
> >>>> + int (*uninit)(AVCodecContext *avctx);
> >>>> +
> >>>> + /**
> >>>> + * Size of the private data to allocate in
> >>>> + * AVCodecInternal.hwaccel_priv_data.
> >>>> + */
> >>>> + int priv_data_size;
> >>>> } AVHWAccel;
> >>>>
> >>>> /**
> >>>> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> >>>> index 64765a2..17de2d5 100644
> >>>> --- a/libavcodec/internal.h
> >>>> +++ b/libavcodec/internal.h
> >>>> @@ -95,6 +95,11 @@ typedef struct AVCodecInternal {
> >>>> * packet into every function.
> >>>> */
> >>>> AVPacket *pkt;
> >>>> +
> >>>> + /**
> >>>> + * hwaccel-specific private data
> >>>> + */
> >>>> + void *hwaccel_priv_data;
> >>>> } AVCodecInternal;
> >>>>
> >>>> struct AVCodecDefault {
> >>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> >>>> index e52d915..5ca7534 100644
> >>>> --- a/libavcodec/utils.c
> >>>> +++ b/libavcodec/utils.c
> >>>> @@ -858,16 +858,37 @@ int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
> >>>> if (!desc)
> >>>> return AV_PIX_FMT_NONE;
> >>>>
> >>>> + if (avctx->hwaccel && avctx->hwaccel->uninit)
> >>>> + avctx->hwaccel->uninit(avctx);
> >>>> + av_freep(&avctx->internal->hwaccel_priv_data);
> >>>> + avctx->hwaccel = NULL;
> >>>> +
> >>>> if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
> >>>> - avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
> >>>> - if (!avctx->hwaccel) {
> >>>> + AVHWAccel *hwaccel;
> >>>> + int err;
> >>>> +
> >>>> + hwaccel = find_hwaccel(avctx->codec_id, ret);
> >>>> + if (!hwaccel) {
> >>>> av_log(avctx, AV_LOG_ERROR,
> >>>> "Could not find an AVHWAccel for the pixel format: %s",
> >>>> desc->name);
> >>>> return AV_PIX_FMT_NONE;
> >>>> }
> >>>> - } else {
> >>>> - avctx->hwaccel = NULL;
> >>>> +
> >>>> + if (hwaccel->priv_data_size) {
> >>>> + avctx->internal->hwaccel_priv_data = av_mallocz(hwaccel->priv_data_size);
> >>>> + if (!avctx->internal->hwaccel_priv_data)
> >>>> + return AV_PIX_FMT_NONE;
> >>>> + }
> >>>> +
> >>>> + if (hwaccel->init) {
> >>>> + err = hwaccel->init(avctx);
> >>>> + if (err < 0) {
> >>>> + av_freep(&avctx->internal->hwaccel_priv_data);
> >>>> + return AV_PIX_FMT_NONE;
> >>>> + }
> >>>> + }
> >>>> + avctx->hwaccel = hwaccel;
> >>>> }
> >>>>
> >>>
> >>> The seems to unconditionally call uninit/init everytime get_format is
> >>> called, is this really required? AFAIK its called more often then
> >>> really required.
> >>> Maybe check if you actually switch a hwaccel on or off, instead of
> >>> just re-initing it all the time?
> >>
> >> The get_format should be called only when there is a dimension/format
> >> change.
> >>
> >
> > "should" is fine, but in my experience it isn't.
>
> I'll look at the codepath to make sure =)
Luca Barbato
2014-03-24 20:20:27 UTC
Permalink
On 24/03/14 20:26, wm4 wrote:
> In practice, it's called when the decoder is flushed, i.e. on seeking.
> And yes, this is a bit of a bother. Freeing all hardware surfaces and
> reallocating them etc. makes seeking quite a bit slower this way.

I'm not sure how to make it more optimal, if you have ideas I'm all hears.

lu
Janne Grunau
2014-03-25 10:13:37 UTC
Permalink
On 2014-03-24 21:20:27 +0100, Luca Barbato wrote:
> On 24/03/14 20:26, wm4 wrote:
> > In practice, it's called when the decoder is flushed, i.e. on seeking.
> > And yes, this is a bit of a bother. Freeing all hardware surfaces and
> > reallocating them etc. makes seeking quite a bit slower this way.
>
> I'm not sure how to make it more optimal, if you have ideas I'm all hears.

I don't think it necessary to call get_format() unconditionally after
seeking. Fixing it in h264.c might be not so nice though.

I guess h264.c is not the only decoder which needs fixing. I'll look
into it.

Janne
Anton Khirnov
2014-04-17 18:38:27 UTC
Permalink
On Tue, 25 Mar 2014 11:13:37 +0100, Janne Grunau <janne-***@jannau.net> wrote:
> On 2014-03-24 21:20:27 +0100, Luca Barbato wrote:
> > On 24/03/14 20:26, wm4 wrote:
> > > In practice, it's called when the decoder is flushed, i.e. on seeking.
> > > And yes, this is a bit of a bother. Freeing all hardware surfaces and
> > > reallocating them etc. makes seeking quite a bit slower this way.
> >
> > I'm not sure how to make it more optimal, if you have ideas I'm all hears.
>
> I don't think it necessary to call get_format() unconditionally after
> seeking. Fixing it in h264.c might be not so nice though.
>
> I guess h264.c is not the only decoder which needs fixing. I'll look
> into it.
>

Any progress?

--
Anton Khirnov
Janne Grunau
2014-04-18 10:27:57 UTC
Permalink
On 2014-04-17 20:38:27 +0200, Anton Khirnov wrote:
>
> On Tue, 25 Mar 2014 11:13:37 +0100, Janne Grunau <janne-***@jannau.net> wrote:
> > On 2014-03-24 21:20:27 +0100, Luca Barbato wrote:
> > > On 24/03/14 20:26, wm4 wrote:
> > > > In practice, it's called when the decoder is flushed, i.e. on seeking.
> > > > And yes, this is a bit of a bother. Freeing all hardware surfaces and
> > > > reallocating them etc. makes seeking quite a bit slower this way.
> > >
> > > I'm not sure how to make it more optimal, if you have ideas I'm all hears.
> >
> > I don't think it necessary to call get_format() unconditionally after
> > seeking. Fixing it in h264.c might be not so nice though.
> >
> > I guess h264.c is not the only decoder which needs fixing. I'll look
> > into it.
> >
>
> Any progress?

Not fully convinced from patch below. Without changes in the stream
get_format will be only called once. That might break applications
using the codec context from avformat_find_stream_info.
One the plus side it's non-ugly and adding support for all other codecs
is a triviality.

Janne

---8<---
The most compelling place to initialize the hardware of AVHWAccel-backed
decoders is the get_format callback. To avoid unneeded reinitializations
ff_get_format() calls get_format() only if the previously returned pixel
format is not in the new pixel format list.
---
libavcodec/h264_slice.c | 10 +++++-----
libavcodec/internal.h | 15 +++++++++++++++
libavcodec/utils.c | 20 ++++++++++++++++++++
3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 897c8eb..683dead 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -1011,11 +1011,11 @@ static enum AVPixelFormat get_pixel_format(H264Context *h)
return h->avctx->color_range == AVCOL_RANGE_JPEG ? AV_PIX_FMT_YUVJ422P
: AV_PIX_FMT_YUV422P;
} else {
- return h->avctx->get_format(h->avctx, h->avctx->codec->pix_fmts ?
- h->avctx->codec->pix_fmts :
- h->avctx->color_range == AVCOL_RANGE_JPEG ?
- h264_hwaccel_pixfmt_list_jpeg_420 :
- h264_hwaccel_pixfmt_list_420);
+ return ff_get_format(h->avctx, h->avctx->codec->pix_fmts ?
+ h->avctx->codec->pix_fmts :
+ h->avctx->color_range == AVCOL_RANGE_JPEG ?
+ h264_hwaccel_pixfmt_list_jpeg_420 :
+ h264_hwaccel_pixfmt_list_420);
}
break;
default:
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index 268a758..10f3530 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -95,6 +95,13 @@ typedef struct AVCodecInternal {
* packet into every function.
*/
AVPacket *pkt;
+
+ /**
+ * AVPixelFormat returned by the last AVCodecContext.get_format() call.
+ * Used to avoid get_format() calls after flush/seeking if the pixel format
+ * has not changed.
+ */
+ enum AVPixelFormat current_pix_fmt;
} AVCodecInternal;

struct AVCodecDefault {
@@ -170,6 +177,14 @@ int ff_get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags);
*/
int ff_reget_buffer(AVCodecContext *avctx, AVFrame *frame);

+/**
+ * Let the user choose the preferred pixel format. This is a wrapper around
+ * AVCodecContext.get_format() and should be used instead calling get_format()
+ * directly when the codec supports AVHWAccels.
+ */
+enum AVPixelFormat ff_get_format(struct AVCodecContext *s,
+ const enum AVPixelFormat *fmt);
+
const uint8_t *avpriv_find_start_code(const uint8_t *restrict p,
const uint8_t *end,
uint32_t *restrict state);
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index d9832e2..203ea8e 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -863,6 +863,25 @@ enum AVPixelFormat avcodec_default_get_format(struct AVCodecContext *s, const en
return fmt[0];
}

+enum AVPixelFormat ff_get_format(struct AVCodecContext *s,
+ const enum AVPixelFormat *fmt)
+{
+ unsigned i = 0;
+
+ if (s->internal->current_pix_fmt != AV_PIX_FMT_NONE &&
+ is_hwaccel_pix_fmt(s->internal->current_pix_fmt)) {
+ while (fmt[i] != AV_PIX_FMT_NONE) {
+ if (fmt[i] == s->internal->current_pix_fmt)
+ return s->internal->current_pix_fmt;
+ i++;
+ }
+ }
+
+ s->internal->current_pix_fmt = s->get_format(s, fmt);
+
+ return s->internal->current_pix_fmt;
+}
+
#if FF_API_AVFRAME_LAVC
void avcodec_get_frame_defaults(AVFrame *frame)
{
@@ -942,6 +961,7 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
ret = AVERROR(ENOMEM);
goto end;
}
+ avctx->internal->current_pix_fmt = AV_PIX_FMT_NONE;

avctx->internal->pool = av_mallocz(sizeof(*avctx->internal->pool));
if (!avctx->internal->pool) {
--
1.9.2
Hendrik Leppkes
2014-04-18 10:37:32 UTC
Permalink
On Fri, Apr 18, 2014 at 12:27 PM, Janne Grunau <janne-***@jannau.net> wrote:
> On 2014-04-17 20:38:27 +0200, Anton Khirnov wrote:
>>
>> On Tue, 25 Mar 2014 11:13:37 +0100, Janne Grunau <janne-***@jannau.net> wrote:
>> > On 2014-03-24 21:20:27 +0100, Luca Barbato wrote:
>> > > On 24/03/14 20:26, wm4 wrote:
>> > > > In practice, it's called when the decoder is flushed, i.e. on seeking.
>> > > > And yes, this is a bit of a bother. Freeing all hardware surfaces and
>> > > > reallocating them etc. makes seeking quite a bit slower this way.
>> > >
>> > > I'm not sure how to make it more optimal, if you have ideas I'm all hears.
>> >
>> > I don't think it necessary to call get_format() unconditionally after
>> > seeking. Fixing it in h264.c might be not so nice though.
>> >
>> > I guess h264.c is not the only decoder which needs fixing. I'll look
>> > into it.
>> >
>>
>> Any progress?
>
> Not fully convinced from patch below. Without changes in the stream
> get_format will be only called once. That might break applications
> using the codec context from avformat_find_stream_info.
> One the plus side it's non-ugly and adding support for all other codecs
> is a triviality.
>

If its supposed to be the all-mighty place to init hwaccels, you also
need to call it when frame size changes, but pixel format stays the
same, so that the hwaccel gets a chance to re-init.
Not sure if some hwaccels have other requirements to be notified of
certain changes.

- Hendrik
Janne Grunau
2014-04-18 13:15:07 UTC
Permalink
The most compelling place to initialize the hardware of AVHWAccel-backed
decoders is the get_format callback. To avoid unneeded reinitializations
ff_get_format() calls get_format() only if the previously returned pixel
format is not in the new pixel format list.

On a frame size change codecs are expected to reset
avctx.internal.current_pix_fmt to AV_PIX_FMT_NONE.
---
libavcodec/h264_slice.c | 15 +++++++++------
libavcodec/internal.h | 15 +++++++++++++++
libavcodec/utils.c | 20 ++++++++++++++++++++
3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index aed2378..dfdc2aa 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -1011,11 +1011,11 @@ static enum AVPixelFormat get_pixel_format(H264Context *h)
return h->avctx->color_range == AVCOL_RANGE_JPEG ? AV_PIX_FMT_YUVJ422P
: AV_PIX_FMT_YUV422P;
} else {
- return h->avctx->get_format(h->avctx, h->avctx->codec->pix_fmts ?
- h->avctx->codec->pix_fmts :
- h->avctx->color_range == AVCOL_RANGE_JPEG ?
- h264_hwaccel_pixfmt_list_jpeg_420 :
- h264_hwaccel_pixfmt_list_420);
+ return ff_get_format(h->avctx, h->avctx->codec->pix_fmts ?
+ h->avctx->codec->pix_fmts :
+ h->avctx->color_range == AVCOL_RANGE_JPEG ?
+ h264_hwaccel_pixfmt_list_jpeg_420 :
+ h264_hwaccel_pixfmt_list_420);
}
break;
default:
@@ -1275,8 +1275,11 @@ int ff_h264_decode_slice_header(H264Context *h, H264Context *h0)
h->avctx->refs = h->sps.ref_frame_count;

if (h->mb_width != h->sps.mb_width ||
- h->mb_height != h->sps.mb_height * (2 - h->sps.frame_mbs_only_flag))
+ h->mb_height != h->sps.mb_height * (2 - h->sps.frame_mbs_only_flag)) {
+ // size changes always need avctx->get_format() call
+ h->avctx->internal->current_pix_fmt = AV_PIX_FMT_NONE;
needs_reinit = 1;
+ }

h->mb_width = h->sps.mb_width;
h->mb_height = h->sps.mb_height * (2 - h->sps.frame_mbs_only_flag);
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index 268a758..10f3530 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -95,6 +95,13 @@ typedef struct AVCodecInternal {
* packet into every function.
*/
AVPacket *pkt;
+
+ /**
+ * AVPixelFormat returned by the last AVCodecContext.get_format() call.
+ * Used to avoid get_format() calls after flush/seeking if the pixel format
+ * has not changed.
+ */
+ enum AVPixelFormat current_pix_fmt;
} AVCodecInternal;

struct AVCodecDefault {
@@ -170,6 +177,14 @@ int ff_get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags);
*/
int ff_reget_buffer(AVCodecContext *avctx, AVFrame *frame);

+/**
+ * Let the user choose the preferred pixel format. This is a wrapper around
+ * AVCodecContext.get_format() and should be used instead calling get_format()
+ * directly when the codec supports AVHWAccels.
+ */
+enum AVPixelFormat ff_get_format(struct AVCodecContext *s,
+ const enum AVPixelFormat *fmt);
+
const uint8_t *avpriv_find_start_code(const uint8_t *restrict p,
const uint8_t *end,
uint32_t *restrict state);
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index d9832e2..203ea8e 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -863,6 +863,25 @@ enum AVPixelFormat avcodec_default_get_format(struct AVCodecContext *s, const en
return fmt[0];
}

+enum AVPixelFormat ff_get_format(struct AVCodecContext *s,
+ const enum AVPixelFormat *fmt)
+{
+ unsigned i = 0;
+
+ if (s->internal->current_pix_fmt != AV_PIX_FMT_NONE &&
+ is_hwaccel_pix_fmt(s->internal->current_pix_fmt)) {
+ while (fmt[i] != AV_PIX_FMT_NONE) {
+ if (fmt[i] == s->internal->current_pix_fmt)
+ return s->internal->current_pix_fmt;
+ i++;
+ }
+ }
+
+ s->internal->current_pix_fmt = s->get_format(s, fmt);
+
+ return s->internal->current_pix_fmt;
+}
+
#if FF_API_AVFRAME_LAVC
void avcodec_get_frame_defaults(AVFrame *frame)
{
@@ -942,6 +961,7 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
ret = AVERROR(ENOMEM);
goto end;
}
+ avctx->internal->current_pix_fmt = AV_PIX_FMT_NONE;

avctx->internal->pool = av_mallocz(sizeof(*avctx->internal->pool));
if (!avctx->internal->pool) {
--
1.9.2
Luca Barbato
2014-04-24 15:00:17 UTC
Permalink
On 18/04/14 15:15, Janne Grunau wrote:
> The most compelling place to initialize the hardware of AVHWAccel-backed
> decoders is the get_format callback. To avoid unneeded reinitializations
> ff_get_format() calls get_format() only if the previously returned pixel
> format is not in the new pixel format list.
>
> On a frame size change codecs are expected to reset
> avctx.internal.current_pix_fmt to AV_PIX_FMT_NONE.
> ---
> libavcodec/h264_slice.c | 15 +++++++++------
> libavcodec/internal.h | 15 +++++++++++++++
> libavcodec/utils.c | 20 ++++++++++++++++++++
> 3 files changed, 44 insertions(+), 6 deletions(-)
>

Could we land this patch and so I can rebase the set against it?

lu
Janne Grunau
2014-04-18 13:24:05 UTC
Permalink
On 2014-04-18 12:37:32 +0200, Hendrik Leppkes wrote:
> On Fri, Apr 18, 2014 at 12:27 PM, Janne Grunau <janne-***@jannau.net> wrote:
> > On 2014-04-17 20:38:27 +0200, Anton Khirnov wrote:
> >>
> >> Any progress?
> >
> > Not fully convinced from patch below. Without changes in the stream
> > get_format will be only called once. That might break applications
> > using the codec context from avformat_find_stream_info.
> > One the plus side it's non-ugly and adding support for all other codecs
> > is a triviality.
>
> If its supposed to be the all-mighty place to init hwaccels, you also
> need to call it when frame size changes, but pixel format stays the
> same, so that the hwaccel gets a chance to re-init.

This is what happens when I pick up incomplete old changes without
thinking why I didn't send them before. thanks for noticing.

> Not sure if some hwaccels have other requirements to be notified of
> certain changes.

profile, levels, number of reference frames changes? BTW how are
pixel bit depth and chroma subsampling changes handled?

>
Luca Barbato
2014-03-24 02:01:51 UTC
Permalink
From: Anton Khirnov <***@khirnov.net>

The current hwaccel is broken and cannot be fixed in a compatible
way. It will be deprecated and replaced with a new one.
---
doc/APIchanges | 3 +++
libavutil/pixdesc.c | 4 ++++
libavutil/pixfmt.h | 1 +
libavutil/version.h | 2 +-
4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index 38d18bc..6d030a7 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -13,6 +13,9 @@ libavutil: 2013-12-xx

API changes, most recent first:

+2014-xx-xx - xxxxxxx - lavu 53.7.0 - pixfmt.h
+ Add AV_PIX_FMT_VDA for new-style VDA acceleration.
+
2014-xx-xx - xxxxxxx - lavu 53.06.0 - pixfmt.h
Add RGBA64 pixel format and variants.

diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
index d0e6919..14dc9f5 100644
--- a/libavutil/pixdesc.c
+++ b/libavutil/pixdesc.c
@@ -1483,6 +1483,10 @@ const AVPixFmtDescriptor av_pix_fmt_descriptors[AV_PIX_FMT_NB] = {
},
.flags = AV_PIX_FMT_FLAG_PLANAR | AV_PIX_FMT_FLAG_BE,
},
+ [AV_PIX_FMT_VDA] = {
+ .name = "vda",
+ .flags = AV_PIX_FMT_FLAG_HWACCEL,
+ },
};

FF_DISABLE_DEPRECATION_WARNINGS
diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
index e86ec7e..870e3cf 100644
--- a/libavutil/pixfmt.h
+++ b/libavutil/pixfmt.h
@@ -196,6 +196,7 @@ enum AVPixelFormat {
AV_PIX_FMT_BGRA64BE, ///< packed RGBA 16:16:16:16, 64bpp, 16B, 16G, 16R, 16A, the 2-byte value for each R/G/B/A component is stored as big-endian
AV_PIX_FMT_BGRA64LE, ///< packed RGBA 16:16:16:16, 64bpp, 16B, 16G, 16R, 16A, the 2-byte value for each R/G/B/A component is stored as little-endian

+ AV_PIX_FMT_VDA, ///< HW acceleration through VDA, data[3] contains a CVPixelBufferRef
AV_PIX_FMT_NB, ///< number of pixel formats, DO NOT USE THIS if you want to link with shared libav* because the number of formats might differ between versions

#if FF_API_PIX_FMT
diff --git a/libavutil/version.h b/libavutil/version.h
index 36070b2..d680979 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -54,7 +54,7 @@
*/

#define LIBAVUTIL_VERSION_MAJOR 53
-#define LIBAVUTIL_VERSION_MINOR 6
+#define LIBAVUTIL_VERSION_MINOR 7
#define LIBAVUTIL_VERSION_MICRO 0

#define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
--
1.8.5.2 (Apple Git-48)
Vittorio Giovara
2014-03-24 02:20:39 UTC
Permalink
On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
> From: Anton Khirnov <***@khirnov.net>
>
> The current hwaccel is broken and cannot be fixed in a compatible
> way. It will be deprecated and replaced with a new one.
> ---
> doc/APIchanges | 3 +++
> libavutil/pixdesc.c | 4 ++++
> libavutil/pixfmt.h | 1 +
> libavutil/version.h | 2 +-
> 4 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 38d18bc..6d030a7 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -13,6 +13,9 @@ libavutil: 2013-12-xx
>
> API changes, most recent first:
>
> +2014-xx-xx - xxxxxxx - lavu 53.7.0 - pixfmt.h
> + Add AV_PIX_FMT_VDA for new-style VDA acceleration.
> +
> 2014-xx-xx - xxxxxxx - lavu 53.06.0 - pixfmt.h
> Add RGBA64 pixel format and variants.
>
> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
> index d0e6919..14dc9f5 100644
> --- a/libavutil/pixdesc.c
> +++ b/libavutil/pixdesc.c
> @@ -1483,6 +1483,10 @@ const AVPixFmtDescriptor av_pix_fmt_descriptors[AV_PIX_FMT_NB] = {
> },
> .flags = AV_PIX_FMT_FLAG_PLANAR | AV_PIX_FMT_FLAG_BE,
> },
> + [AV_PIX_FMT_VDA] = {
> + .name = "vda",
> + .flags = AV_PIX_FMT_FLAG_HWACCEL,
> + },
> };

no .log2_chroma_w/h?

Vittorio
Luca Barbato
2014-03-24 05:09:58 UTC
Permalink
On 24/03/14 03:20, Vittorio Giovara wrote:
> On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
>> From: Anton Khirnov <***@khirnov.net>
>>
>> The current hwaccel is broken and cannot be fixed in a compatible
>> way. It will be deprecated and replaced with a new one.
>> ---
>> doc/APIchanges | 3 +++
>> libavutil/pixdesc.c | 4 ++++
>> libavutil/pixfmt.h | 1 +
>> libavutil/version.h | 2 +-
>> 4 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 38d18bc..6d030a7 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -13,6 +13,9 @@ libavutil: 2013-12-xx
>>
>> API changes, most recent first:
>>
>> +2014-xx-xx - xxxxxxx - lavu 53.7.0 - pixfmt.h
>> + Add AV_PIX_FMT_VDA for new-style VDA acceleration.
>> +
>> 2014-xx-xx - xxxxxxx - lavu 53.06.0 - pixfmt.h
>> Add RGBA64 pixel format and variants.
>>
>> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
>> index d0e6919..14dc9f5 100644
>> --- a/libavutil/pixdesc.c
>> +++ b/libavutil/pixdesc.c
>> @@ -1483,6 +1483,10 @@ const AVPixFmtDescriptor av_pix_fmt_descriptors[AV_PIX_FMT_NB] = {
>> },
>> .flags = AV_PIX_FMT_FLAG_PLANAR | AV_PIX_FMT_FLAG_BE,
>> },
>> + [AV_PIX_FMT_VDA] = {
>> + .name = "vda",
>> + .flags = AV_PIX_FMT_FLAG_HWACCEL,
>> + },
>> };
>
> no .log2_chroma_w/h?
>

Monsieur de Lapalice states:
"Opaque pixel formats are opaque"

I'll add a note about it in the wiki soon.

lu
Rémi Denis-Courmont
2014-03-24 08:31:41 UTC
Permalink
On Mon, 24 Mar 2014 06:09:58 +0100, Luca Barbato <***@gentoo.org>
wrote:
>> no .log2_chroma_w/h?
>>
>
> Monsieur de Lapalice states:
> "Opaque pixel formats are opaque"
>
> I'll add a note about it in the wiki soon.

Then again, the current hardware surface pixel formats are all 4:2:0
8-bits YUV, aren't they? Presumably new pixel formats would be required to
unambiguously convey another chroma sampling or depth to the get_format()
implementation.

--
Rémi Denis-Courmont
Sent from my collocated server
Luca Barbato
2014-03-24 09:09:45 UTC
Permalink
On 24/03/14 09:31, Rémi Denis-Courmont wrote:
> On Mon, 24 Mar 2014 06:09:58 +0100, Luca Barbato <***@gentoo.org>
> wrote:
>>> no .log2_chroma_w/h?
>>>
>>
>> Monsieur de Lapalice states:
>> "Opaque pixel formats are opaque"
>>
>> I'll add a note about it in the wiki soon.
>
> Then again, the current hardware surface pixel formats are all 4:2:0
> 8-bits YUV, aren't they?

In case of VDA and VT it could be anything from UYVY422 to YUV420Planar
to ...

> Presumably new pixel formats would be required to
> unambiguously convey another chroma sampling or depth to the
> get_format() implementation.

Nope, opaque is opaque, you know nothing of what's is inside and the
opaque must be able to describe itself.

lu
Rémi Denis-Courmont
2014-03-24 09:22:13 UTC
Permalink
On Mon, 24 Mar 2014 10:09:45 +0100, Luca Barbato <***@gentoo.org>
wrote:
> On 24/03/14 09:31, Rémi Denis-Courmont wrote:
>> On Mon, 24 Mar 2014 06:09:58 +0100, Luca Barbato <***@gentoo.org>
>> wrote:
>>>> no .log2_chroma_w/h?
>>>>
>>>
>>> Monsieur de Lapalice states:
>>> "Opaque pixel formats are opaque"
>>>
>>> I'll add a note about it in the wiki soon.
>>
>> Then again, the current hardware surface pixel formats are all 4:2:0
>> 8-bits YUV, aren't they?
>
> In case of VDA and VT it could be anything from UYVY422

Last I checked, libavcodec did not support hardware acceleration for
anything other than 4:2:0 - just looking at the pixel format lists...
Anyway, is there even *real* hardware slice-level decoder that supports
4:2:2? As far as I know, hardware video acceleration frameworks support
4:2:2 for post-processing and rendering, not (really yet) for decoding.

> to YUV420Planar to ...

There are several cases whereby the application needs to know the
underlying type of a surface:
1) A function is the acceleration framework requests it as a parameter.
2) The application wants to copy the surface content back to main memory.
3) The application wants to show the chroma sampling as meta-data.
4) The application exports the surface via interoperability (e.g. to
OpenGL).

Hence, it seems more reasonable to allocate separate pixel formats for
separate samplings and depths... if the situation ever arises.

>> Presumably new pixel formats would be required to
>> unambiguously convey another chroma sampling or depth to the
>> get_format() implementation.
>
> Nope, opaque is opaque, you know nothing of what's is inside and the
> opaque must be able to describe itself.

Sorry but that is crap. The application typically knows (and needs to
know) the surface size. And sometimes it also needs to know the colour
space, chroma sampling and composent depth. It is one thing that the
surface content is not directly accessible. It is a completely different
thing to know nothing of the surface properties.

--
Rémi Denis-Courmont
Sent from my collocated server
Luca Barbato
2014-03-24 09:44:48 UTC
Permalink
On 24/03/14 10:22, Rémi Denis-Courmont wrote:
> On Mon, 24 Mar 2014 10:09:45 +0100, Luca Barbato <***@gentoo.org>
>> In case of VDA and VT it could be anything from UYVY422
>
> Last I checked, libavcodec did not support hardware acceleration for
> anything other than 4:2:0 - just looking at the pixel format lists...
> Anyway, is there even *real* hardware slice-level decoder that supports
> 4:2:2? As far as I know, hardware video acceleration frameworks support
> 4:2:2 for post-processing and rendering, not (really yet) for decoding.

I'm playing with abstractions that use the bitstream and not the slice.

>> to YUV420Planar to ...
>
> There are several cases whereby the application needs to know the
> underlying type of a surface:
> 1) A function is the acceleration framework requests it as a parameter.
> 2) The application wants to copy the surface content back to main memory.
> 3) The application wants to show the chroma sampling as meta-data.
> 4) The application exports the surface via interoperability (e.g. to
> OpenGL).
>
> Hence, it seems more reasonable to allocate separate pixel formats for
> separate samplings and depths... if the situation ever arises.

The application will know asking the opaque delivered by the means
provided by the underlying protocol/whatnot.

>>> Presumably new pixel formats would be required to
>>> unambiguously convey another chroma sampling or depth to the
>>> get_format() implementation.
>>
>> Nope, opaque is opaque, you know nothing of what's is inside and the
>> opaque must be able to describe itself.
>
> Sorry but that is crap. The application typically knows (and needs to
> know) the surface size. And sometimes it also needs to know the colour
> space, chroma sampling and composent depth. It is one thing that the
> surface content is not directly accessible. It is a completely different
> thing to know nothing of the surface properties.

bitstream/slice is fed to a blackbox that outputs opaque data in unknown
format.

The application then can render/manipulate the opaque by the means
provided by the blackbox and outside Libav.

hwaccel2 will optionally provide generic abstractions to manipulate the
opaque (render them, probe them for the format and such).

All we do currently is to wrap the opaque in an avframe and make sure it
behaves as expected (e.g. refcounting works on seeks and such).

lu
Anton Khirnov
2014-03-24 19:23:05 UTC
Permalink
On Mon, 24 Mar 2014 10:22:13 +0100, Rémi Denis-Courmont <***@remlab.net> wrote:
> On Mon, 24 Mar 2014 10:09:45 +0100, Luca Barbato <***@gentoo.org>
> wrote:
> > On 24/03/14 09:31, Rémi Denis-Courmont wrote:
> >> On Mon, 24 Mar 2014 06:09:58 +0100, Luca Barbato <***@gentoo.org>
> >> wrote:
> >>>> no .log2_chroma_w/h?
> >>>>
> >>>
> >>> Monsieur de Lapalice states:
> >>> "Opaque pixel formats are opaque"
> >>>
> >>> I'll add a note about it in the wiki soon.
> >>
> >> Then again, the current hardware surface pixel formats are all 4:2:0
> >> 8-bits YUV, aren't they?
> >
> > In case of VDA and VT it could be anything from UYVY422
>
> Last I checked, libavcodec did not support hardware acceleration for
> anything other than 4:2:0 - just looking at the pixel format lists...
> Anyway, is there even *real* hardware slice-level decoder that supports
> 4:2:2? As far as I know, hardware video acceleration frameworks support
> 4:2:2 for post-processing and rendering, not (really yet) for decoding.
>

Just because we do not currently support them does not strike me as a good
reason to limit our API so we _cannot_ support them in the future.

> > to YUV420Planar to ...
>
> There are several cases whereby the application needs to know the
> underlying type of a surface:
> 1) A function is the acceleration framework requests it as a parameter.
> 2) The application wants to copy the surface content back to main memory.
> 3) The application wants to show the chroma sampling as meta-data.
> 4) The application exports the surface via interoperability (e.g. to
> OpenGL).
>

In all the current cases other than new VDA with magic wrappers, the application
explicitly chooses the underlying format it will use.
In all the current cases the application can call some function to get the
underlying format.

So I really don't see what would be the advantage in restricting the API.

--
Anton Khirnov
Rémi Denis-Courmont
2014-03-24 20:16:01 UTC
Permalink
Le lundi 24 mars 2014, 20:23:05 Anton Khirnov a écrit :
> > Last I checked, libavcodec did not support hardware acceleration for
> > anything other than 4:2:0 - just looking at the pixel format lists...
> > Anyway, is there even *real* hardware slice-level decoder that supports
> > 4:2:2? As far as I know, hardware video acceleration frameworks support
> > 4:2:2 for post-processing and rendering, not (really yet) for decoding.
>
> Just because we do not currently support them does not strike me as a good
> reason to limit our API so we _cannot_ support them in the future.

The point is that currently those surface types are used by 4:2:0 surfaces
only. If support for 4:2:2 or Hi10p or whatever gets added to libavcodec, the
newly used surface types should be assigned new pixel formats, consistently
with the underlying acceleration APIs as well as with libavcodec software
pixel formats.

It would suck for applications to have to second-guess libavcodec.

> > > to YUV420Planar to ...
> >
> > There are several cases whereby the application needs to know the
> > underlying type of a surface:
> > 1) A function is the acceleration framework requests it as a parameter.
> > 2) The application wants to copy the surface content back to main memory.
> > 3) The application wants to show the chroma sampling as meta-data.
> > 4) The application exports the surface via interoperability (e.g. to
> > OpenGL).
>
> In all the current cases other than new VDA with magic wrappers, the
> application explicitly chooses the underlying format it will use.
> In all the current cases the application can call some function to get the
> underlying format.
>
> So I really don't see what would be the advantage in restricting the API.

This does not restrict the API in any way, quite the contrary.

--
Rémi Denis-Courmont
http://www.remlab.net/
Hendrik Leppkes
2014-03-24 20:24:47 UTC
Permalink
On Mon, Mar 24, 2014 at 9:16 PM, Rémi Denis-Courmont <***@remlab.net> wrote:
> Le lundi 24 mars 2014, 20:23:05 Anton Khirnov a écrit :
>> > Last I checked, libavcodec did not support hardware acceleration for
>> > anything other than 4:2:0 - just looking at the pixel format lists...
>> > Anyway, is there even *real* hardware slice-level decoder that supports
>> > 4:2:2? As far as I know, hardware video acceleration frameworks support
>> > 4:2:2 for post-processing and rendering, not (really yet) for decoding.
>>
>> Just because we do not currently support them does not strike me as a good
>> reason to limit our API so we _cannot_ support them in the future.
>
> The point is that currently those surface types are used by 4:2:0 surfaces
> only. If support for 4:2:2 or Hi10p or whatever gets added to libavcodec, the
> newly used surface types should be assigned new pixel formats, consistently
> with the underlying acceleration APIs as well as with libavcodec software
> pixel formats.
>
> It would suck for applications to have to second-guess libavcodec.
>

IMHO, it would suck for applications to need yet another mapping table
to map surface types to pixel formats, when one pixel format that says
"there is a hardware surface in there" is all thats needed.
For most HWAccels, avcodec just doesn't need to know, nor should it know.

If I take DXVA2 as an example, since thats what I know best, the
application negotiates with the GPU driver which format is used, which
can in theory be anything. In practice, most GPUs support NV12, some
support YV12 additionally.
avcodec does not care, nor should it care, which format I negotiated
for with the driver. It just passes the opaque surfaces around, from
the user app to the hardware decoder, and back again.

Adding separate pixel formats just bloats up the list here with no
real-word usefulness.

- Hendrik
Rémi Denis-Courmont
2014-03-24 20:40:30 UTC
Permalink
Le lundi 24 mars 2014, 21:24:47 Hendrik Leppkes a écrit :
> > The point is that currently those surface types are used by 4:2:0 surfaces
> > only. If support for 4:2:2 or Hi10p or whatever gets added to libavcodec,
> > the newly used surface types should be assigned new pixel formats,
> > consistently with the underlying acceleration APIs as well as with
> > libavcodec software pixel formats.
> >
> > It would suck for applications to have to second-guess libavcodec.
>
> IMHO, it would suck for applications to need yet another mapping table
> to map surface types to pixel formats, when one pixel format that says
> "there is a hardware surface in there" is all thats needed.

I don't think it would suck. But most importantly, what you propose would
*break* existing applications, both at source and binary level. This is not
acceptable.

> For most HWAccels, avcodec just doesn't need to know, nor should it know.
>
> If I take DXVA2 as an example, since thats what I know best, the
> application negotiates with the GPU driver which format is used, which
> can in theory be anything. In practice, most GPUs support NV12, some
> support YV12 additionally.

DxVA2 negotiates the format while instantiating the decoder, but I somewhat
doubt that the drivers will actually do any good if the application does not
match the chroma sampling manually. In practice, applications just work
because so far, device drivers only ever exposed 4:2:0 8-bits such that there
is no way to hit a mismatch. But I would hardly be surprised if DxVA2
applications started breaking if/when 4:2:2 or Hi10p-capable hardware comes
out.

> avcodec does not care, nor should it care, which format I negotiated
> for with the driver. It just passes the opaque surfaces around, from
> the user app to the hardware decoder, and back again.

avcodec *has* to care because it the application needs to know this. This is
needed for VA-API's vaCreateSurfaces() and VDPAU's VdpVideoSurfaceCreate to
work, period.

Not only that, but it obviously affects semantics when dumping the frame to
main memory, or when exporting it to OpenGL/EGL/OpenCL/CUDA/NVENC/QSV/whatever
via interoperability functions.

--
Rémi Denis-Courmont
http://www.remlab.net/
Hendrik Leppkes
2014-03-24 20:46:44 UTC
Permalink
On Mon, Mar 24, 2014 at 9:40 PM, Rémi Denis-Courmont <***@remlab.net> wrote:
> Le lundi 24 mars 2014, 21:24:47 Hendrik Leppkes a écrit :
>> > The point is that currently those surface types are used by 4:2:0 surfaces
>> > only. If support for 4:2:2 or Hi10p or whatever gets added to libavcodec,
>> > the newly used surface types should be assigned new pixel formats,
>> > consistently with the underlying acceleration APIs as well as with
>> > libavcodec software pixel formats.
>> >
>> > It would suck for applications to have to second-guess libavcodec.
>>
>> IMHO, it would suck for applications to need yet another mapping table
>> to map surface types to pixel formats, when one pixel format that says
>> "there is a hardware surface in there" is all thats needed.
>
> I don't think it would suck. But most importantly, what you propose would
> *break* existing applications, both at source and binary level. This is not
> acceptable.
>
>> For most HWAccels, avcodec just doesn't need to know, nor should it know.
>>
>> If I take DXVA2 as an example, since thats what I know best, the
>> application negotiates with the GPU driver which format is used, which
>> can in theory be anything. In practice, most GPUs support NV12, some
>> support YV12 additionally.
>
> DxVA2 negotiates the format while instantiating the decoder, but I somewhat
> doubt that the drivers will actually do any good if the application does not
> match the chroma sampling manually. In practice, applications just work
> because so far, device drivers only ever exposed 4:2:0 8-bits such that there
> is no way to hit a mismatch. But I would hardly be surprised if DxVA2
> applications started breaking if/when 4:2:2 or Hi10p-capable hardware comes
> out.
>
>> avcodec does not care, nor should it care, which format I negotiated
>> for with the driver. It just passes the opaque surfaces around, from
>> the user app to the hardware decoder, and back again.
>
> avcodec *has* to care because it the application needs to know this. This is
> needed for VA-API's vaCreateSurfaces() and VDPAU's VdpVideoSurfaceCreate to
> work, period.
>
> Not only that, but it obviously affects semantics when dumping the frame to
> main memory, or when exporting it to OpenGL/EGL/OpenCL/CUDA/NVENC/QSV/whatever
> via interoperability functions.
>

But this is all stuff the user app calls, *not* avcodec. If you create
the surface in some format, you better remember which format that was
when you interact with it after decoding again.
avcodec does *not* interact with the surface, therefor it does *not*
need to know which format it is in.

- Hendrik
Rémi Denis-Courmont
2014-03-24 20:54:42 UTC
Permalink
Le lundi 24 mars 2014, 21:46:44 Hendrik Leppkes a écrit :
> But this is all stuff the user app calls, *not* avcodec. If you create
> the surface in some format, you better remember which format that was
> when you interact with it after decoding again.

And the application knows the correct surface type how?

Duh.

--
Rémi Denis-Courmont
http://www.remlab.net/
wm4
2014-03-24 20:48:35 UTC
Permalink
On Mon, 24 Mar 2014 22:40:30 +0200
Rémi Denis-Courmont <***@remlab.net> wrote:

> Le lundi 24 mars 2014, 21:24:47 Hendrik Leppkes a écrit :
> > > The point is that currently those surface types are used by 4:2:0 surfaces
> > > only. If support for 4:2:2 or Hi10p or whatever gets added to libavcodec,
> > > the newly used surface types should be assigned new pixel formats,
> > > consistently with the underlying acceleration APIs as well as with
> > > libavcodec software pixel formats.
> > >
> > > It would suck for applications to have to second-guess libavcodec.
> >
> > IMHO, it would suck for applications to need yet another mapping table
> > to map surface types to pixel formats, when one pixel format that says
> > "there is a hardware surface in there" is all thats needed.
>
> I don't think it would suck. But most importantly, what you propose would
> *break* existing applications, both at source and binary level. This is not
> acceptable.

How? You create decoder and surfaces. It's entirely under your control.

> > For most HWAccels, avcodec just doesn't need to know, nor should it know.
> >
> > If I take DXVA2 as an example, since thats what I know best, the
> > application negotiates with the GPU driver which format is used, which
> > can in theory be anything. In practice, most GPUs support NV12, some
> > support YV12 additionally.
>
> DxVA2 negotiates the format while instantiating the decoder, but I somewhat
> doubt that the drivers will actually do any good if the application does not
> match the chroma sampling manually. In practice, applications just work
> because so far, device drivers only ever exposed 4:2:0 8-bits such that there
> is no way to hit a mismatch. But I would hardly be surprised if DxVA2
> applications started breaking if/when 4:2:2 or Hi10p-capable hardware comes
> out.
>
> > avcodec does not care, nor should it care, which format I negotiated
> > for with the driver. It just passes the opaque surfaces around, from
> > the user app to the hardware decoder, and back again.
>
> avcodec *has* to care because it the application needs to know this. This is
> needed for VA-API's vaCreateSurfaces() and VDPAU's VdpVideoSurfaceCreate to
> work, period.
>
> Not only that, but it obviously affects semantics when dumping the frame to
> main memory, or when exporting it to OpenGL/EGL/OpenCL/CUDA/NVENC/QSV/whatever
> via interoperability functions.

Then just carry the real format as part as your own frame structure
(your equivalent of AVFrame), and negotiate it using your own pixel
formats. I don't think VLC uses the libav ones natively. So the status
quo is that you can do all these things just fine.
Anton Khirnov
2014-03-24 20:27:34 UTC
Permalink
On Mon, 24 Mar 2014 22:16:01 +0200, Rémi Denis-Courmont <***@remlab.net> wrote:
> Le lundi 24 mars 2014, 20:23:05 Anton Khirnov a écrit :
> > > Last I checked, libavcodec did not support hardware acceleration for
> > > anything other than 4:2:0 - just looking at the pixel format lists...
> > > Anyway, is there even *real* hardware slice-level decoder that supports
> > > 4:2:2? As far as I know, hardware video acceleration frameworks support
> > > 4:2:2 for post-processing and rendering, not (really yet) for decoding.
> >
> > Just because we do not currently support them does not strike me as a good
> > reason to limit our API so we _cannot_ support them in the future.
>
> The point is that currently those surface types are used by 4:2:0 surfaces
> only. If support for 4:2:2 or Hi10p or whatever gets added to libavcodec, the
> newly used surface types should be assigned new pixel formats, consistently
> with the underlying acceleration APIs as well as with libavcodec software
> pixel formats.
>
> It would suck for applications to have to second-guess libavcodec.
>

What would be the advantage in having separate pixel formats. A normal pixel
format tells you how is the data laid out in memory and how you are supposed to
inrepret it. A hwaccel pixel format just tells you that the data is outside the
scope of libav and you have to deal with it using a special external API. You
would still use the same API whether it's 8bit 420 or 10bit 444. So from the
point of view of libav it's still the same format.

--
Anton Khirnov
Rémi Denis-Courmont
2014-03-24 20:42:32 UTC
Permalink
Le lundi 24 mars 2014, 21:27:34 Anton Khirnov a écrit :
> On Mon, 24 Mar 2014 22:16:01 +0200, Rémi Denis-Courmont <***@remlab.net>
wrote:
> > Le lundi 24 mars 2014, 20:23:05 Anton Khirnov a écrit :
> > > > Last I checked, libavcodec did not support hardware acceleration for
> > > > anything other than 4:2:0 - just looking at the pixel format lists...
> > > > Anyway, is there even *real* hardware slice-level decoder that
> > > > supports
> > > > 4:2:2? As far as I know, hardware video acceleration frameworks
> > > > support
> > > > 4:2:2 for post-processing and rendering, not (really yet) for
> > > > decoding.
> > >
> > > Just because we do not currently support them does not strike me as a
> > > good
> > > reason to limit our API so we _cannot_ support them in the future.
> >
> > The point is that currently those surface types are used by 4:2:0 surfaces
> > only. If support for 4:2:2 or Hi10p or whatever gets added to libavcodec,
> > the newly used surface types should be assigned new pixel formats,
> > consistently with the underlying acceleration APIs as well as with
> > libavcodec software pixel formats.
> >
> > It would suck for applications to have to second-guess libavcodec.
>
> What would be the advantage in having separate pixel formats. A normal pixel
> format tells you how is the data laid out in memory and how you are
> supposed to inrepret it.

You don't need to know how the surface is packed on the GPU, but you obviously
do need to know the chroma sampling and the bit depth to allocate the surface,
in addition to the pixel width and height.

You also do need to know that in the 4 cases outlined in a previous email.

> A hwaccel pixel format just tells you that the
> data is outside the scope of libav and you have to deal with it using a
> special external API. You would still use the same API whether it's 8bit
> 420 or 10bit 444. So from the point of view of libav it's still the same
> format.

Same API, different parameter. That's exactly what pixel formats are for.

--
Rémi Denis-Courmont
http://www.remlab.net/
Luca Barbato
2014-03-24 20:29:02 UTC
Permalink
On 24/03/14 21:16, Rémi Denis-Courmont wrote:
> The point is that currently those surface types are used by 4:2:0 surfaces
> only. If support for 4:2:2 or Hi10p or whatever gets added to libavcodec, the
> newly used surface types should be assigned new pixel formats, consistently
> with the underlying acceleration APIs as well as with libavcodec software
> pixel formats.
>
> It would suck for applications to have to second-guess libavcodec.

libavcodec was originally supposed NOT to know anything (the global
context wasn't even set by it), thus the assumption to know nothing
about the output frame accordingly.

With hwaccel1.2 you have a set of default setup and destroy functions,
so the output frame is more or less set (depending if the setup function
takes args changing it or not), but for the "traditional" hwaccel the
AVFrame is totally opaque and the additional fields got cargo-culted in.

With hwaccel2 you get the option of even not having to take care of the
rendering if you want it to happen in system memory.

With avscale you get the option to have the scaling/conversion happen in
hardware (if the hardware supports it).

lu
Rémi Denis-Courmont
2014-03-24 20:47:12 UTC
Permalink
Le lundi 24 mars 2014, 21:29:02 Luca Barbato a écrit :
> On 24/03/14 21:16, Rémi Denis-Courmont wrote:
> > The point is that currently those surface types are used by 4:2:0 surfaces
> > only. If support for 4:2:2 or Hi10p or whatever gets added to libavcodec,
> > the newly used surface types should be assigned new pixel formats,
> > consistently with the underlying acceleration APIs as well as with
> > libavcodec software pixel formats.
> >
> > It would suck for applications to have to second-guess libavcodec.
>
> libavcodec was originally supposed NOT to know anything (the global
> context wasn't even set by it), thus the assumption to know nothing
> about the output frame accordingly.

Go check the VA-API and VDPAU header files and documentation, instead of
barraging me with the same nonsensical arguments over and over. Whoever
allocates the surfaces MUST KNOW the chroma sampling and pixel depth for
OBVIOUS REASONS.

If you keep a single pixel format regardless, the only way for the application
to know the correct parameters is look at the last pixel format (i.e.
software) in the list and second-guess libavcodec that this indicates the
correct chroma type and pixel depth. This would be vomitively ugly, and it
would forever lock libavcodec into offering only a single software output pixel
format ever. Is that really what you want?

--
Rémi Denis-Courmont
http://www.remlab.net/
Luca Barbato
2014-03-24 20:53:35 UTC
Permalink
On 24/03/14 21:47, Rémi Denis-Courmont wrote:
> Le lundi 24 mars 2014, 21:29:02 Luca Barbato a écrit :
>> On 24/03/14 21:16, Rémi Denis-Courmont wrote:
>>> The point is that currently those surface types are used by 4:2:0 surfaces
>>> only. If support for 4:2:2 or Hi10p or whatever gets added to libavcodec,
>>> the newly used surface types should be assigned new pixel formats,
>>> consistently with the underlying acceleration APIs as well as with
>>> libavcodec software pixel formats.
>>>
>>> It would suck for applications to have to second-guess libavcodec.
>>
>> libavcodec was originally supposed NOT to know anything (the global
>> context wasn't even set by it), thus the assumption to know nothing
>> about the output frame accordingly.
>
> Go check the VA-API and VDPAU header files and documentation, instead of
> barraging me with the same nonsensical arguments over and over. Whoever
> allocates the surfaces MUST KNOW the chroma sampling and pixel depth for
> OBVIOUS REASONS.

First friendly warning: please tone down two notches.

> If you keep a single pixel format regardless, the only way for the application
> to know the correct parameters is look at the last pixel format (i.e.
> software) in the list and second-guess libavcodec that this indicates the
> correct chroma type and pixel depth. This would be vomitively ugly, and it
> would forever lock libavcodec into offering only a single software output pixel
> format ever. Is that really what you want?

It is actually what normally happens, as explained by Hendrik, Anton and
wm4.

I'm sorry about this misconception, sadly hwaccel hadn't been documented
so everybody had an understanding on how it works and all the time a
detail or another might fit by pure chance.

lu
Rémi Denis-Courmont
2014-03-24 21:09:04 UTC
Permalink
Le lundi 24 mars 2014, 21:53:35 Luca Barbato a écrit :
> First friendly warning: please tone down two notches.

I suggest you check your facts before you flame me then taunt me.

> > If you keep a single pixel format regardless, the only way for the
> > application to know the correct parameters is look at the last pixel
> > format (i.e. software) in the list and second-guess libavcodec that this
> > indicates the correct chroma type and pixel depth. This would be
> > vomitively ugly, and it would forever lock libavcodec into offering only
> > a single software output pixel format ever. Is that really what you want?
>
> It is actually what normally happens, as explained by Hendrik, Anton and
> wm4.

This is patently false. Look at get_pixel_format() from h264.c for yourself!

libavcodec does not currently call get_format() for anything other than 420
content(*)- Therefore, it does not offer AV_PIX_FMT_(VDPAU|VAAPI) for 422.
Instead, it just hard codes the software decoding pixel format in that case.

> I'm sorry about this misconception, sadly hwaccel hadn't been documented
> so everybody had an understanding on how it works and all the time a
> detail or another might fit by pure chance.

You are distorting reality here. This stuff (non-420 hwaccel) is not
implemented yet. Of the two potential ways to implement it, you defend the
backward-incompatible and ugliest one.

(*) with the irrelevant exception of the 8BPS decoder.

--
Rémi
Luca Barbato
2014-03-24 21:25:35 UTC
Permalink
On 24/03/14 22:09, Rémi Denis-Courmont wrote:
> Le lundi 24 mars 2014, 21:53:35 Luca Barbato a écrit :
>> First friendly warning: please tone down two notches.
>
> I suggest you check your facts before you flame me then taunt me.

Nobody is flaming you, just this kind of tone is not welcome.

>>> If you keep a single pixel format regardless, the only way for the
>>> application to know the correct parameters is look at the last pixel
>>> format (i.e. software) in the list and second-guess libavcodec that this
>>> indicates the correct chroma type and pixel depth. This would be
>>> vomitively ugly, and it would forever lock libavcodec into offering only
>>> a single software output pixel format ever. Is that really what you want?
>>
>> It is actually what normally happens, as explained by Hendrik, Anton and
>> wm4.
>
> This is patently false. Look at get_pixel_format() from h264.c for yourself!

The code in avconv_vdpau and mpv does the surface mapping based on the
global context (set by the very same application).

VDA does the same.

> libavcodec does not currently call get_format() for anything other than 420
> content(*)- Therefore, it does not offer AV_PIX_FMT_(VDPAU|VAAPI) for 422.
> Instead, it just hard codes the software decoding pixel format in that case.

That can be easily fixed, I guess.

>> I'm sorry about this misconception, sadly hwaccel hadn't been documented
>> so everybody had an understanding on how it works and all the time a
>> detail or another might fit by pure chance.
>
> You are distorting reality here. This stuff (non-420 hwaccel) is not
> implemented yet. Of the two potential ways to implement it, you defend the
> backward-incompatible and ugliest one.

I'm not distorting anything, please tone down.

You assume one way is the correct one, the other people involved assume
the other.

As I told you, the original hwaccel does not assume anything beside
black boxes and opaque pointers.

What Hendrik, Anton, wm4 consider correct as approach had been
implemented on their side, you assumed otherwise and implemented that
way in VLC.

It can be *calmly* discussed w/out considering a war.

Worst case I'll fix that part in VLC if you do not want to.

lu
wm4
2014-03-24 21:34:35 UTC
Permalink
On Mon, 24 Mar 2014 22:25:35 +0100
Luca Barbato <***@gentoo.org> wrote:

> On 24/03/14 22:09, Rémi Denis-Courmont wrote:
> > Le lundi 24 mars 2014, 21:53:35 Luca Barbato a écrit :
> >> First friendly warning: please tone down two notches.
> >
> > I suggest you check your facts before you flame me then taunt me.
>
> Nobody is flaming you, just this kind of tone is not welcome.
>
> >>> If you keep a single pixel format regardless, the only way for the
> >>> application to know the correct parameters is look at the last pixel
> >>> format (i.e. software) in the list and second-guess libavcodec that this
> >>> indicates the correct chroma type and pixel depth. This would be
> >>> vomitively ugly, and it would forever lock libavcodec into offering only
> >>> a single software output pixel format ever. Is that really what you want?
> >>
> >> It is actually what normally happens, as explained by Hendrik, Anton and
> >> wm4.
> >
> > This is patently false. Look at get_pixel_format() from h264.c for yourself!
>
> The code in avconv_vdpau and mpv does the surface mapping based on the
> global context (set by the very same application).
>
> VDA does the same.

I think the problem at hand is that there's no way for the hwaccel API
to signal to the application which surface format should be used.

And he's right: if new surface types other than 420 were ever
supported, it would probably break dxva/vdpau/vaapi.

So the basic problem is that even if the decoder reports a higher
level profile as supported, the application (written against the
current API) would allocate the wrong surface type.

> > libavcodec does not currently call get_format() for anything other than 420
> > content(*)- Therefore, it does not offer AV_PIX_FMT_(VDPAU|VAAPI) for 422.
> > Instead, it just hard codes the software decoding pixel format in that case.
>
> That can be easily fixed, I guess.
>
> >> I'm sorry about this misconception, sadly hwaccel hadn't been documented
> >> so everybody had an understanding on how it works and all the time a
> >> detail or another might fit by pure chance.
> >
> > You are distorting reality here. This stuff (non-420 hwaccel) is not
> > implemented yet. Of the two potential ways to implement it, you defend the
> > backward-incompatible and ugliest one.
>
> I'm not distorting anything, please tone down.
>
> You assume one way is the correct one, the other people involved assume
> the other.
>
> As I told you, the original hwaccel does not assume anything beside
> black boxes and opaque pointers.
>
> What Hendrik, Anton, wm4 consider correct as approach had been
> implemented on their side, you assumed otherwise and implemented that
> way in VLC.
>
> It can be *calmly* discussed w/out considering a war.
>
> Worst case I'll fix that part in VLC if you do not want to.
>
> lu
Luca Barbato
2014-03-24 21:39:10 UTC
Permalink
On 24/03/14 22:34, wm4 wrote:
> On Mon, 24 Mar 2014 22:25:35 +0100
> Luca Barbato <***@gentoo.org> wrote:
>
>> On 24/03/14 22:09, Rémi Denis-Courmont wrote:
>>> Le lundi 24 mars 2014, 21:53:35 Luca Barbato a écrit :
>>>> First friendly warning: please tone down two notches.
>>>
>>> I suggest you check your facts before you flame me then taunt me.
>>
>> Nobody is flaming you, just this kind of tone is not welcome.
>>
>>>>> If you keep a single pixel format regardless, the only way for the
>>>>> application to know the correct parameters is look at the last pixel
>>>>> format (i.e. software) in the list and second-guess libavcodec that this
>>>>> indicates the correct chroma type and pixel depth. This would be
>>>>> vomitively ugly, and it would forever lock libavcodec into offering only
>>>>> a single software output pixel format ever. Is that really what you want?
>>>>
>>>> It is actually what normally happens, as explained by Hendrik, Anton and
>>>> wm4.
>>>
>>> This is patently false. Look at get_pixel_format() from h264.c for yourself!
>>
>> The code in avconv_vdpau and mpv does the surface mapping based on the
>> global context (set by the very same application).
>>
>> VDA does the same.
>
> I think the problem at hand is that there's no way for the hwaccel API
> to signal to the application which surface format should be used.
>

Mostly because it was assumed it doesn't know at all.

You configure the decoder, you feed it the surfaces. All hwaccel was
doing was just feed the decoder the slices and get back the opaque
frames to you.

lu
wm4
2014-03-24 21:44:51 UTC
Permalink
On Mon, 24 Mar 2014 22:39:10 +0100
Luca Barbato <***@gentoo.org> wrote:

> On 24/03/14 22:34, wm4 wrote:
> > On Mon, 24 Mar 2014 22:25:35 +0100
> > Luca Barbato <***@gentoo.org> wrote:
> >
> >> On 24/03/14 22:09, Rémi Denis-Courmont wrote:
> >>> Le lundi 24 mars 2014, 21:53:35 Luca Barbato a écrit :
> >>>> First friendly warning: please tone down two notches.
> >>>
> >>> I suggest you check your facts before you flame me then taunt me.
> >>
> >> Nobody is flaming you, just this kind of tone is not welcome.
> >>
> >>>>> If you keep a single pixel format regardless, the only way for the
> >>>>> application to know the correct parameters is look at the last pixel
> >>>>> format (i.e. software) in the list and second-guess libavcodec that this
> >>>>> indicates the correct chroma type and pixel depth. This would be
> >>>>> vomitively ugly, and it would forever lock libavcodec into offering only
> >>>>> a single software output pixel format ever. Is that really what you want?
> >>>>
> >>>> It is actually what normally happens, as explained by Hendrik, Anton and
> >>>> wm4.
> >>>
> >>> This is patently false. Look at get_pixel_format() from h264.c for yourself!
> >>
> >> The code in avconv_vdpau and mpv does the surface mapping based on the
> >> global context (set by the very same application).
> >>
> >> VDA does the same.
> >
> > I think the problem at hand is that there's no way for the hwaccel API
> > to signal to the application which surface format should be used.
> >
>
> Mostly because it was assumed it doesn't know at all.
>
> You configure the decoder, you feed it the surfaces. All hwaccel was
> doing was just feed the decoder the slices and get back the opaque
> frames to you.

Yes, but you still need to create the surface. Now assume you created a
decoder that expects 422 surfaces, and you give it 420 surfaces. It
will probably break. And the hwaccel API user basically can't know
which format it should use to create surfaces.

So yes, I'm starting to see a problem here too. It's not a current
problem because everything assumes 420, but as hardware decoders gain
more features, it could become one.
Rémi Denis-Courmont
2014-03-25 18:16:35 UTC
Permalink
Le lundi 24 mars 2014, 22:44:51 wm4 a écrit :
> > You configure the decoder, you feed it the surfaces. All hwaccel was
> > doing was just feed the decoder the slices and get back the opaque
> > frames to you.

> Yes, but you still need to create the surface. Now assume you created a
> decoder that expects 422 surfaces, and you give it 420 surfaces. It
> will probably break. And the hwaccel API user basically can't know
> which format it should use to create surfaces.
>
> So yes, I'm starting to see a problem here too. It's not a current
> problem because everything assumes 420, but as hardware decoders gain
> more features, it could become one.

Though 422 would presumably be a lot less invasive to the hardware design than
9/10-bits, still extra transistors mean larger die which means higher costs...
This would only make sense if there were a big market push for 422.

I am actually more concerned about get_format() passing more than one software
pixel format at once, than about hardware acceleration supporting 422, in the
future. If the choice were between NV12 and YV12, there would be no ambiguity
regarding the bits depth and colour subsampling. But if it were between 8-bits
and 10-bits (say because a codec supported high-depth as an enhancement layer)
or between 420 and 422, then using "the" software pixel format as reference
would no longer work. In fact, it would no longer mean anything.

My view is that so long as get_format() is used to select hardware
acceleration, it needs provision to convey the surface type(s) and select it
if there are more than one possibilities. There may be other ways to achieve
it than multiplying pixel formats, but overloading the meaning of "the"
software pixel format within the list does not seem very robust or future-
proof.

--
Rémi Denis-Courmont
http://www.remlab.net/
Rémi Denis-Courmont
2014-03-24 21:51:58 UTC
Permalink
Le lundi 24 mars 2014, 22:25:35 Luca Barbato a écrit :
> On 24/03/14 22:09, Rémi Denis-Courmont wrote:
> > Le lundi 24 mars 2014, 21:53:35 Luca Barbato a écrit :
> >> First friendly warning: please tone down two notches.
> >
> > I suggest you check your facts before you flame me then taunt me.
>
> Nobody is flaming you, just this kind of tone is not welcome.
>
> >>> If you keep a single pixel format regardless, the only way for the
> >>> application to know the correct parameters is look at the last pixel
> >>> format (i.e. software) in the list and second-guess libavcodec that this
> >>> indicates the correct chroma type and pixel depth. This would be
> >>> vomitively ugly, and it would forever lock libavcodec into offering only
> >>> a single software output pixel format ever. Is that really what you
> >>> want?
> >>
> >> It is actually what normally happens, as explained by Hendrik, Anton and
> >> wm4.
> >
> > This is patently false. Look at get_pixel_format() from h264.c for
> > yourself!
> The code in avconv_vdpau and mpv does the surface mapping based on the
> global context (set by the very same application).
>
> VDA does the same.
>
> > libavcodec does not currently call get_format() for anything other than
> > 420
> > content(*)- Therefore, it does not offer AV_PIX_FMT_(VDPAU|VAAPI) for 422.
> > Instead, it just hard codes the software decoding pixel format in that
> > case.
> That can be easily fixed, I guess.
>
> >> I'm sorry about this misconception, sadly hwaccel hadn't been documented
> >> so everybody had an understanding on how it works and all the time a
> >> detail or another might fit by pure chance.
> >
> > You are distorting reality here. This stuff (non-420 hwaccel) is not
> > implemented yet. Of the two potential ways to implement it, you defend the
> > backward-incompatible and ugliest one.
>
> I'm not distorting anything, please tone down.
>
> You assume one way is the correct one, the other people involved assume
> the other.
>
> As I told you, the original hwaccel does not assume anything beside
> black boxes and opaque pointers.
>
> What Hendrik, Anton, wm4 consider correct as approach had been
> implemented on their side, you assumed otherwise and implemented that
> way in VLC.

There are about zero benefits to using the same pixel formats for different
chroma types in VDPAU and VAAPI (maybe for DxVA2, I doubt it, but I don't
really care). If you add new

> It can be *calmly* discussed w/out considering a war.
>
> Worst case I'll fix that part in VLC if you do not want to.

The VLC VAAPI code has been doing that (so far correct) assumption for three
major releases already, and VDPAU for one. Given that even Anton made the same
assumption for avconv_vdpau in libav 10 release too, I consider it is part of
the interface promise for the VDPAU pixel format.

This is not a matter of fixing VLC. This is a compatibility issue. You'd have
to create one new pixel format in the enumeration to avoid breaking
compatibility anyway (like this series does for VDA). You might just as well
map pixel format to chroma types. It is cleaner, much more difficult to
implement wrongly, and consistent with the design of libvdpau and libva.

--
Rémi Denis-Courmont
http://www.remlab.net/
Luca Barbato
2014-03-24 21:59:29 UTC
Permalink
On 24/03/14 22:51, Rémi Denis-Courmont wrote:
> This is not a matter of fixing VLC. This is a compatibility issue. You'd have
> to create one new pixel format in the enumeration to avoid breaking
> compatibility anyway (like this series does for VDA). You might just as well
> map pixel format to chroma types. It is cleaner, much more difficult to
> implement wrongly, and consistent with the design of libvdpau and libva.

As said before, can be discussed.

In itself you should get the information in get_format and use it in
get_buffer,
how is up to discussion (possibly not an heated one) since it hadn't
been defined before.

lu
Rémi Denis-Courmont
2014-03-25 08:24:03 UTC
Permalink
On Mon, 24 Mar 2014 22:59:29 +0100, Luca Barbato <***@gentoo.org>
wrote:
> As said before, can be discussed.

That's not what you said before. You said I was wrong and misconstrued an
hypothetical future implementation design choice as an established existing
fact for justification.

--
Rémi Denis-Courmont
Sent from my collocated server
wm4
2014-03-24 19:32:39 UTC
Permalink
On Mon, 24 Mar 2014 10:22:13 +0100
Rémi Denis-Courmont <***@remlab.net> wrote:

> On Mon, 24 Mar 2014 10:09:45 +0100, Luca Barbato <***@gentoo.org>
> wrote:
> > On 24/03/14 09:31, Rémi Denis-Courmont wrote:
> >> On Mon, 24 Mar 2014 06:09:58 +0100, Luca Barbato <***@gentoo.org>
> >> wrote:
> >>>> no .log2_chroma_w/h?
> >>>>
> >>>
> >>> Monsieur de Lapalice states:
> >>> "Opaque pixel formats are opaque"
> >>>
> >>> I'll add a note about it in the wiki soon.
> >>
> >> Then again, the current hardware surface pixel formats are all 4:2:0
> >> 8-bits YUV, aren't they?
> >
> > In case of VDA and VT it could be anything from UYVY422
>
> Last I checked, libavcodec did not support hardware acceleration for
> anything other than 4:2:0 - just looking at the pixel format lists...
> Anyway, is there even *real* hardware slice-level decoder that supports
> 4:2:2? As far as I know, hardware video acceleration frameworks support
> 4:2:2 for post-processing and rendering, not (really yet) for decoding.
>
> > to YUV420Planar to ...
>
> There are several cases whereby the application needs to know the
> underlying type of a surface:
> 1) A function is the acceleration framework requests it as a parameter.
> 2) The application wants to copy the surface content back to main memory.
> 3) The application wants to show the chroma sampling as meta-data.
> 4) The application exports the surface via interoperability (e.g. to
> OpenGL).
>
> Hence, it seems more reasonable to allocate separate pixel formats for
> separate samplings and depths... if the situation ever arises.

My code in theory supports different hardware formats (although in
practice it's always 420, and anything else wasn't useful yet). I
simply store the format separately. That is no problem, because with all
hwaccels (except VDA) you allocate the surfaces yourself. VDA allocates
surfaces for you, but I'm sure CVPixelBufferRefs let you easily query
the underlying format.

> >> Presumably new pixel formats would be required to
> >> unambiguously convey another chroma sampling or depth to the
> >> get_format() implementation.
> >
> > Nope, opaque is opaque, you know nothing of what's is inside and the
> > opaque must be able to describe itself.
>
> Sorry but that is crap. The application typically knows (and needs to
> know) the surface size. And sometimes it also needs to know the colour
> space, chroma sampling and composent depth. It is one thing that the
> surface content is not directly accessible. It is a completely different
> thing to know nothing of the surface properties.
>
Luca Barbato
2014-03-24 20:23:22 UTC
Permalink
On 24/03/14 20:32, wm4 wrote:
> My code in theory supports different hardware formats (although in
> practice it's always 420, and anything else wasn't useful yet). I
> simply store the format separately. That is no problem, because with all
> hwaccels (except VDA) you allocate the surfaces yourself. VDA allocates
> surfaces for you, but I'm sure CVPixelBufferRefs let you easily query
> the underlying format.

See the avconv_vda.c for an example, the format of the CVPixelBufferRefs
had been set since both the main users (you and vlc) seem to favour UVYV
(and works decently).

lu
Rémi Denis-Courmont
2014-03-24 20:32:33 UTC
Permalink
Le lundi 24 mars 2014, 20:32:39 wm4 a écrit :
> > > to YUV420Planar to ...
> >
> > There are several cases whereby the application needs to know the
> > underlying type of a surface:
> > 1) A function is the acceleration framework requests it as a parameter.
> > 2) The application wants to copy the surface content back to main memory.
> > 3) The application wants to show the chroma sampling as meta-data.
> > 4) The application exports the surface via interoperability (e.g. to
> > OpenGL).
> >
> > Hence, it seems more reasonable to allocate separate pixel formats for
> > separate samplings and depths... if the situation ever arises.
>
> My code in theory supports different hardware formats (although in
> practice it's always 420, and anything else wasn't useful yet).

With VDPAU, VLC supports 4:2:2 already (and it has even been tested
succesfully for post-processing and rendering). But it uses different pixel
formats for 4:2:2 than 4:2:0.

If suddenly libavcodec started offering AV_PIX_FMT_VDPAU for 4:2:2, decoding
would break. VLC would not know that suddenly a pixel format that was 4:2:0
has become 4:2:2 (too). It would therefore allocate VDP_CHROMA_TYPE_420
surfaces from the hardware. That is obviously wrong and a backward
compatibility breakage.

The correct approach, should the need arise, would be to define
AV_PIX_FMT_VDPAU_422 or whatever. Then old VLC versions would ignore and fall
back to software decoding as before. New versions can add support safely.

I don't know how it is with VDA. But both VDPAU and VA-API work that way. And
it is not like the libavutil pixel format enumeration is short on numbering
space.

--
Rémi Denis-Courmont
http://www.remlab.net/
Luca Barbato
2014-03-24 20:46:54 UTC
Permalink
On 24/03/14 21:32, Rémi Denis-Courmont wrote:
> Le lundi 24 mars 2014, 20:32:39 wm4 a écrit :
>>>> to YUV420Planar to ...
>>>
>>> There are several cases whereby the application needs to know the
>>> underlying type of a surface:
>>> 1) A function is the acceleration framework requests it as a parameter.
>>> 2) The application wants to copy the surface content back to main memory.
>>> 3) The application wants to show the chroma sampling as meta-data.
>>> 4) The application exports the surface via interoperability (e.g. to
>>> OpenGL).
>>>
>>> Hence, it seems more reasonable to allocate separate pixel formats for
>>> separate samplings and depths... if the situation ever arises.
>>
>> My code in theory supports different hardware formats (although in
>> practice it's always 420, and anything else wasn't useful yet).
>
> With VDPAU, VLC supports 4:2:2 already (and it has even been tested
> succesfully for post-processing and rendering). But it uses different pixel
> formats for 4:2:2 than 4:2:0.

> If suddenly libavcodec started offering AV_PIX_FMT_VDPAU for 4:2:2, decoding
> would break. VLC would not know that suddenly a pixel format that was 4:2:0
> has become 4:2:2 (too).

Nothing would happen "suddenly", you set in the global context which is
the pixel format (so you know by the time your callback is it in
get_format).

> It would therefore allocate VDP_CHROMA_TYPE_420
> surfaces from the hardware. That is obviously wrong and a backward
> compatibility breakage.
>
> The correct approach, should the need arise, would be to define
> AV_PIX_FMT_VDPAU_422 or whatever. Then old VLC versions would ignore and fall
> back to software decoding as before. New versions can add support safely.

No, and you got 3 different explanations why it is wrong.

> I don't know how it is with VDA. But both VDPAU and VA-API work that way.

vdpau according to the code I'm seeing for avconv and mpv does work as
I'm stating:

- on get_format you figure out what you need and set it in the global
context.
- on get_buffer you provide the buffer using the correct VDP_
(CHROMA_TYPE|YCBCR_FORMAT).

Nothing breaks and nothing would break even by moving to hwaccel2 while
keeping a single pixel format for hwaccel.

Probably would be easier if I get you a patchset for vdpau (libav and
vlc) so what I mean is clear.

lu
Rémi Denis-Courmont
2014-03-24 21:19:59 UTC
Permalink
Le lundi 24 mars 2014, 21:46:54 Luca Barbato a écrit :
> > If suddenly libavcodec started offering AV_PIX_FMT_VDPAU for 4:2:2,
> > decoding would break. VLC would not know that suddenly a pixel format
> > that was 4:2:0 has become 4:2:2 (too).
>
> Nothing would happen "suddenly", you set in the global context which is
> the pixel format (so you know by the time your callback is it in
> get_format).

Oh yes it will break.

The hardware might be able to decode a 420 bitstream into a 422 surface. But
it definitely will not be able to decode a 422 bitstream into a 420 surface,
because the memory allocation will be too small.

Thus all applications that assume that AV_PIX_FMT_VDPAU or AV_PIX_FMT_VAAPI is
420 will break. Even your own avconv_vdpau will break!

> > It would therefore allocate VDP_CHROMA_TYPE_420
> > surfaces from the hardware. That is obviously wrong and a backward
> > compatibility breakage.
> >
> > The correct approach, should the need arise, would be to define
> > AV_PIX_FMT_VDPAU_422 or whatever. Then old VLC versions would ignore and
> > fall back to software decoding as before. New versions can add support
> > safely.
> No, and you got 3 different explanations why it is wrong.

The only explanation I got was that it is possible for an application to work
around the problem by checking the last pixel format. That is an ugly,
limiting work-around and does not solve backward compatibility in any way.

> vdpau according to the code I'm seeing for avconv and mpv does work as
> I'm stating:

As a matter of fact, the avconv_vdpau code hardcodes 420 chroma type both in
hardware capability check and in surfaces allocation. If you ever feed it
non-420 bitstream, it will break just as badly as VLC.

> - on get_format you figure out what you need and set it in the global
> context.
> - on get_buffer you provide the buffer using the correct VDP_
> (CHROMA_TYPE|YCBCR_FORMAT).

The read-back code supports 422, but the surface allocation code does not and
will break. For ****'s sake, there is not a single mention of
VDP_CHROMA_TYPE_422 in the entire libav tree.

> Nothing breaks and nothing would break even by moving to hwaccel2 while
> keeping a single pixel format for hwaccel.

Oh yes things break.

--
Rémi Denis-Courmont
http://www.remlab.net/
Luca Barbato
2014-03-24 21:36:29 UTC
Permalink
On 24/03/14 22:19, Rémi Denis-Courmont wrote:
> Le lundi 24 mars 2014, 21:46:54 Luca Barbato a écrit :
>>> If suddenly libavcodec started offering AV_PIX_FMT_VDPAU for 4:2:2,
>>> decoding would break. VLC would not know that suddenly a pixel format
>>> that was 4:2:0 has become 4:2:2 (too).
>>
>> Nothing would happen "suddenly", you set in the global context which is
>> the pixel format (so you know by the time your callback is it in
>> get_format).
>
> Oh yes it will break.
>
> The hardware might be able to decode a 420 bitstream into a 422 surface. But
> it definitely will not be able to decode a 422 bitstream into a 420 surface,
> because the memory allocation will be too small.

Who is asking the hardware to do the decoding is the same setting the
surface is the same software.

> Thus all applications that assume that AV_PIX_FMT_VDPAU or AV_PIX_FMT_VAAPI is
> 420 will break. Even your own avconv_vdpau will break!

If it does there is a bug and will be fixed.

>>> It would therefore allocate VDP_CHROMA_TYPE_420
>>> surfaces from the hardware. That is obviously wrong and a backward
>>> compatibility breakage.
>>>
>>> The correct approach, should the need arise, would be to define
>>> AV_PIX_FMT_VDPAU_422 or whatever. Then old VLC versions would ignore and
>>> fall back to software decoding as before. New versions can add support
>>> safely.
>> No, and you got 3 different explanations why it is wrong.
>
> The only explanation I got was that it is possible for an application to work
> around the problem by checking the last pixel format. That is an ugly,
> limiting work-around and does not solve backward compatibility in any way.

What I told you is that libavcodec knows _nothing_ about that since both
the global context setup and the get buffer is on the user.

I'm trying to move most of the boilerplate code back in libav so then
your approach should be considered (but then the backward compatibility
will bite me in the form of a stabbing Anton and a slicing Hendrik).

>> vdpau according to the code I'm seeing for avconv and mpv does work as
>> I'm stating:
>
> As a matter of fact, the avconv_vdpau code hardcodes 420 chroma type both in
> hardware capability check and in surfaces allocation. If you ever feed it
> non-420 bitstream, it will break just as badly as VLC.

Then should be fixed, I was looking at this mapping

static const int vdpau_formats[][2] = {
{ VDP_YCBCR_FORMAT_YV12, AV_PIX_FMT_YUV420P },
{ VDP_YCBCR_FORMAT_NV12, AV_PIX_FMT_NV12 },
{ VDP_YCBCR_FORMAT_YUYV, AV_PIX_FMT_YUYV422 },
{ VDP_YCBCR_FORMAT_UYVY, AV_PIX_FMT_UYVY422 },
};


And

switch (imgfmt) {
case IMGFMT_420P:
ycbcr = VDP_YCBCR_FORMAT_YV12;
break;
case IMGFMT_NV12:
ycbcr = VDP_YCBCR_FORMAT_NV12;
break;
case IMGFMT_YUYV:
ycbcr = VDP_YCBCR_FORMAT_YUYV;
chroma = VDP_CHROMA_TYPE_422;
break;
case IMGFMT_UYVY:
ycbcr = VDP_YCBCR_FORMAT_UYVY;
chroma = VDP_CHROMA_TYPE_422;
break;

This one.

I have no qualms in being wrong or correct, I'm just telling what I
gather after I discussed this topic for quite a bit of time and having a
look at the code.

>> - on get_format you figure out what you need and set it in the global
>> context.
>> - on get_buffer you provide the buffer using the correct VDP_
>> (CHROMA_TYPE|YCBCR_FORMAT).
>
> The read-back code supports 422, but the surface allocation code does not and
> will break. For ****'s sake, there is not a single mention of
> VDP_CHROMA_TYPE_422 in the entire libav tree.

There is no need to use colorful language.

>> Nothing breaks and nothing would break even by moving to hwaccel2 while
>> keeping a single pixel format for hwaccel.
>
> Oh yes things break.

Then we'll fix them, we are good at it =)

lu
Rémi Denis-Courmont
2014-03-24 21:51:53 UTC
Permalink
Le lundi 24 mars 2014, 22:36:29 Luca Barbato a écrit :
> On 24/03/14 22:19, Rémi Denis-Courmont wrote:
> > Le lundi 24 mars 2014, 21:46:54 Luca Barbato a écrit :
> >>> If suddenly libavcodec started offering AV_PIX_FMT_VDPAU for 4:2:2,
> >>> decoding would break. VLC would not know that suddenly a pixel format
> >>> that was 4:2:0 has become 4:2:2 (too).
> >>
> >> Nothing would happen "suddenly", you set in the global context which is
> >> the pixel format (so you know by the time your callback is it in
> >> get_format).
> >
> > Oh yes it will break.
> >
> > The hardware might be able to decode a 420 bitstream into a 422 surface.
> > But it definitely will not be able to decode a 422 bitstream into a 420
> > surface, because the memory allocation will be too small.
>
> Who is asking the hardware to do the decoding is the same setting the
> surface is the same software.
>
> > Thus all applications that assume that AV_PIX_FMT_VDPAU or
> > AV_PIX_FMT_VAAPI is 420 will break. Even your own avconv_vdpau will
> > break!
>
> If it does there is a bug and will be fixed.

That is not a bugfix. That is a redefinition of semantics after the fact in
incompatible ways, a.k.a. silent ABI and API break.

AV_PIX_FMT_VAAPI and AV_PIX_FMT_VDPAU mean 4:2:0 8-bits YUV so far. If you
want to change that, you need to define new enumeration values and/or a
get_format2() callback. Otherwise that is a silent API and ABI breakage.

> >>> It would therefore allocate VDP_CHROMA_TYPE_420
> >>> surfaces from the hardware. That is obviously wrong and a backward
> >>> compatibility breakage.
> >>>
> >>> The correct approach, should the need arise, would be to define
> >>> AV_PIX_FMT_VDPAU_422 or whatever. Then old VLC versions would ignore and
> >>> fall back to software decoding as before. New versions can add support
> >>> safely.
> >>
> >> No, and you got 3 different explanations why it is wrong.
> >
> > The only explanation I got was that it is possible for an application to
> > work around the problem by checking the last pixel format. That is an
> > ugly, limiting work-around and does not solve backward compatibility in
> > any way.
> What I told you is that libavcodec knows _nothing_ about that since both
> the global context setup and the get buffer is on the user.

What I keep telling you is that the application needs to know the chroma type,
and that the one that knows it is libavcodec.

I agree that libavcodec does not need to know the chroma type so far as it is
not allocation the surfaces. But it needs to convey the chroma type to the
application, and the pixel format seem to be the obvious way to achieve it.

--
Rémi Denis-Courmont
http://www.remlab.net/
wm4
2014-03-24 22:09:21 UTC
Permalink
On Mon, 24 Mar 2014 23:51:53 +0200
Rémi Denis-Courmont <***@remlab.net> wrote:

> Le lundi 24 mars 2014, 22:36:29 Luca Barbato a écrit :
> > On 24/03/14 22:19, Rémi Denis-Courmont wrote:
> > > Le lundi 24 mars 2014, 21:46:54 Luca Barbato a écrit :
> > >>> If suddenly libavcodec started offering AV_PIX_FMT_VDPAU for 4:2:2,
> > >>> decoding would break. VLC would not know that suddenly a pixel format
> > >>> that was 4:2:0 has become 4:2:2 (too).
> > >>
> > >> Nothing would happen "suddenly", you set in the global context which is
> > >> the pixel format (so you know by the time your callback is it in
> > >> get_format).
> > >
> > > Oh yes it will break.
> > >
> > > The hardware might be able to decode a 420 bitstream into a 422 surface.
> > > But it definitely will not be able to decode a 422 bitstream into a 420
> > > surface, because the memory allocation will be too small.
> >
> > Who is asking the hardware to do the decoding is the same setting the
> > surface is the same software.
> >
> > > Thus all applications that assume that AV_PIX_FMT_VDPAU or
> > > AV_PIX_FMT_VAAPI is 420 will break. Even your own avconv_vdpau will
> > > break!
> >
> > If it does there is a bug and will be fixed.
>
> That is not a bugfix. That is a redefinition of semantics after the fact in
> incompatible ways, a.k.a. silent ABI and API break.
>
> AV_PIX_FMT_VAAPI and AV_PIX_FMT_VDPAU mean 4:2:0 8-bits YUV so far. If you
> want to change that, you need to define new enumeration values and/or a
> get_format2() callback. Otherwise that is a silent API and ABI breakage.
>
> > >>> It would therefore allocate VDP_CHROMA_TYPE_420
> > >>> surfaces from the hardware. That is obviously wrong and a backward
> > >>> compatibility breakage.
> > >>>
> > >>> The correct approach, should the need arise, would be to define
> > >>> AV_PIX_FMT_VDPAU_422 or whatever. Then old VLC versions would ignore and
> > >>> fall back to software decoding as before. New versions can add support
> > >>> safely.
> > >>
> > >> No, and you got 3 different explanations why it is wrong.
> > >
> > > The only explanation I got was that it is possible for an application to
> > > work around the problem by checking the last pixel format. That is an
> > > ugly, limiting work-around and does not solve backward compatibility in
> > > any way.
> > What I told you is that libavcodec knows _nothing_ about that since both
> > the global context setup and the get buffer is on the user.
>
> What I keep telling you is that the application needs to know the chroma type,
> and that the one that knows it is libavcodec.
>
> I agree that libavcodec does not need to know the chroma type so far as it is
> not allocation the surfaces. But it needs to convey the chroma type to the
> application, and the pixel format seem to be the obvious way to achieve it.

Not sure about this. Different decoders might support different pixel
formats for the same thing. For example, it seems VDA always prefers
packed 422, even for 420. Likewise, there could be decoders which
support hi10p decoding, but pretend the surfaces are normal 420 8 bit
surfaces. (There was something about adding 10 bit support for vdpau
some months ago. I'm not sure how that was supposed to work or if it
was just a misunderstanding, but it looked like it was pretending to
return 8 bit surfaces for 10 bit decoding.)

Is it really that straight-forward? Maybe separating this would be
better. We don't store the codec profile in the pixel format either.
And when we did that, it was ugly and we eventually removed it (like
AV_PIX_FMT_VDPAU_H264).

IMO it's best if AV_PIX_FMT_VDPAU means that it's a VdpVideoSurface,
instead of attempting to encode more into it.
Rémi Denis-Courmont
2014-03-25 08:43:17 UTC
Permalink
On Mon, 24 Mar 2014 23:09:21 +0100, wm4 <***@googlemail.com> wrote:
> Not sure about this. Different decoders might support different pixel
> formats for the same thing.

I never said that the pixel formats should expose the internal packing.

> For example, it seems VDA always prefers packed 422, even for 420.

I would say that is irrelevant. If VDA always uses 422, then you might as
well define AV_PIX_FMT_VDA to mean 422. If VDA completely hides the chroma
sampling from the application, then I agree that a single AV_PIX_FMT_VDA
for
all uses is good. However, the presence of cv_pix_fmt_type in the AV VDA
context makes me suspect that VDA does in fact care about the chroma
sampling just like the other hardware accelerators do.

In any case, VA and VDPAU require the application to know the chroma type
(and, so far, they assume 8-bits per component) for the purposes of
surface allocation, interoperability and copy to memory. Hence libavcodec
needs to supply that information. The pixel format seems like the obvious
way to do so. And even if another new mechanism were defined, the
_current_ VA and VDPAU pixel formats would not be compatible with it.

On the other hand, VA and VDPAU do not require the application to know or
care about the actual packing (say NV12 vs YV12, YUYV vs UYVY), so I agree
that there is no point in exposing the packing through the pixel format,
or through any other mean. I never asked for _that_.

DxVA2 is another matter altogether as it does not seem to define a fixed
set of surface types.

> Likewise, there could be decoders which
> support hi10p decoding, but pretend the surfaces are normal 420 8 bit
> surfaces.

All I'm saying is that libavcodec should match libva and libvdpau (and
consequently VLC). If those libraries reuse the currently implicitly
8-bits chroma types for 10-bits, then I am totally fine with reusing the
existing libav pixel formats accordingly.

In practice, this is extremely unlikely though because of the memory
allocation would be too small to fit the extra bits.

> (There was something about adding 10 bit support for vdpau
> some months ago. I'm not sure how that was supposed to work or if it
> was just a misunderstanding, but it looked like it was pretending to
> return 8 bit surfaces for 10 bit decoding.)

IMU, that would have used new video surface chroma types. But it never
came to fruition as the Gallium guys never posted any patches of any sort.
VDPAU already defines separate 8-bits and 10-bits types for RGB, that is
to say output surfaces in VDPAU parliance.

> Is it really that straight-forward? Maybe separating this would be
> better. We don't store the codec profile in the pixel format either.

The codec profile is stored in the AVCodecContext. I am fine with storing
the sampling there, but that does not seem to fit well with the
get_format() pattern.

> And when we did that, it was ugly and we eventually removed it (like
> AV_PIX_FMT_VDPAU_H264).

Indeed, the input codec is irrelevant to the VDPAU video surface. But the
chroma type is very relevant to the VDPAU video surface.

> IMO it's best if AV_PIX_FMT_VDPAU means that it's a VdpVideoSurface,
> instead of attempting to encode more into it.

The point is, there is not one type of VDPAU video surface. There are
three types of them, of which two are actively used. To preserve
compatibility, a new pixel format would be required to support 422 anyway.

With that in mind, is it better to have simply:

AV_PIX_FMT_VDPAU_420,
#define AV_PIX_FMT_VDPAU AV_PIX_FMT_VDPAU_420
/*...*/
AV_PIX_FMT_VDPAU_422,

or

AV_PIX_FMT_VDPAU,
/* ... */
AV_PIX_FMT_VDPAU2,
/* ... */

VdpChromaType av_vdpau2_get_chroma_type(const AVPixelFormat *pix_fmts)
{
/* Lot of code here */
}

Either way, you end up with two pixel formats. I think it the first option
is cleaner, saner and simpler.

--
Rémi Denis-Courmont
Sent from my collocated server
Vittorio Giovara
2014-03-24 16:15:58 UTC
Permalink
On Mon, Mar 24, 2014 at 6:09 AM, Luca Barbato <***@gentoo.org> wrote:
> On 24/03/14 03:20, Vittorio Giovara wrote:
>> On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
>>> From: Anton Khirnov <***@khirnov.net>
>>>
>>> The current hwaccel is broken and cannot be fixed in a compatible
>>> way. It will be deprecated and replaced with a new one.
>>> ---
>>> doc/APIchanges | 3 +++
>>> libavutil/pixdesc.c | 4 ++++
>>> libavutil/pixfmt.h | 1 +
>>> libavutil/version.h | 2 +-
>>> 4 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index 38d18bc..6d030a7 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -13,6 +13,9 @@ libavutil: 2013-12-xx
>>>
>>> API changes, most recent first:
>>>
>>> +2014-xx-xx - xxxxxxx - lavu 53.7.0 - pixfmt.h
>>> + Add AV_PIX_FMT_VDA for new-style VDA acceleration.
>>> +
>>> 2014-xx-xx - xxxxxxx - lavu 53.06.0 - pixfmt.h
>>> Add RGBA64 pixel format and variants.
>>>
>>> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
>>> index d0e6919..14dc9f5 100644
>>> --- a/libavutil/pixdesc.c
>>> +++ b/libavutil/pixdesc.c
>>> @@ -1483,6 +1483,10 @@ const AVPixFmtDescriptor av_pix_fmt_descriptors[AV_PIX_FMT_NB] = {
>>> },
>>> .flags = AV_PIX_FMT_FLAG_PLANAR | AV_PIX_FMT_FLAG_BE,
>>> },
>>> + [AV_PIX_FMT_VDA] = {
>>> + .name = "vda",
>>> + .flags = AV_PIX_FMT_FLAG_HWACCEL,
>>> + },
>>> };
>>
>> no .log2_chroma_w/h?
>>
>
> Monsieur de Lapalice states:
> "Opaque pixel formats are opaque"
>

True that, but the other opaque formats have those, so either remove
the chroma subsampling from all of them or add those here, imho ;)
--
Vittorio
Rémi Denis-Courmont
2014-03-24 16:41:03 UTC
Permalink
Le lundi 24 mars 2014, 17:15:58 Vittorio Giovara a écrit :
> True that, but the other opaque formats have those, so either remove
> the chroma subsampling from all of them

No. The values are correct for VDPAU, VA and DxVA2.

--
Rémi Denis-Courmont
http://www.remlab.net/
Luca Barbato
2014-03-24 17:37:06 UTC
Permalink
On 24/03/14 17:15, Vittorio Giovara wrote:
> True that, but the other opaque formats have those, so either remove
> the chroma subsampling from all of them or add those here, imho ;)

I'd rather not let users assume something that could change (the
avconv_vda.c in fact checks the pixel format on purpose).

lu
wm4
2014-03-24 20:04:09 UTC
Permalink
On Mon, 24 Mar 2014 03:01:51 +0100
Luca Barbato <***@gentoo.org> wrote:


> + AV_PIX_FMT_VDA, ///< HW acceleration through VDA, data[3] contains a CVPixelBufferRef

I keep wondering about this: why data[3]? data[0] would be a little bit
less strange. Is this due to historical reasons (hw surfaces can be 0,
and data[0] being NULL did not work due to now removed checks), or
consistency with older hwaccels, or something else?
Luca Barbato
2014-03-24 20:32:45 UTC
Permalink
On 24/03/14 21:04, wm4 wrote:
> On Mon, 24 Mar 2014 03:01:51 +0100
> Luca Barbato <***@gentoo.org> wrote:
>
>
>> + AV_PIX_FMT_VDA, ///< HW acceleration through VDA, data[3] contains a CVPixelBufferRef
>
> I keep wondering about this: why data[3]? data[0] would be a little bit
> less strange. Is this due to historical reasons (hw surfaces can be 0,
> and data[0] being NULL did not work due to now removed checks), or
> consistency with older hwaccels, or something else?

Mostly consistency with older hwaccels, sadly.

lu
Luca Barbato
2014-03-24 02:01:52 UTC
Permalink
From: Anton Khirnov <***@khirnov.net>

---
libavcodec/vda.h | 6 +++---
libavcodec/vda_h264.c | 50 +++++++++++++++++++++++++++++++++++---------------
2 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..1189e41 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -112,17 +112,17 @@ struct vda_context {
OSType cv_pix_fmt_type;

/**
- * The current bitstream buffer.
+ * unused
*/
uint8_t *priv_bitstream;

/**
- * The current size of the bitstream.
+ * unused
*/
int priv_bitstream_size;

/**
- * The reference size used for fast reallocation.
+ * unused
*/
int priv_allocated_size;
};
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..72b0c78 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -28,6 +28,17 @@
#include "h264.h"
#include "vda.h"

+typedef struct VDAContext {
+ // The current bitstream buffer.
+ uint8_t *bitstream;
+
+ // The current size of the bitstream.
+ int bitstream_size;
+
+ // The reference size used for fast reallocation.
+ int allocated_size;
+} VDAContext;
+
/* Decoder callback that adds the VDA frame to the queue in display order. */
static void vda_decoder_callback(void *vda_hw_ctx,
CFDictionaryRef user_info,
@@ -46,15 +57,15 @@ static void vda_decoder_callback(void *vda_hw_ctx,
vda_ctx->cv_buffer = CVPixelBufferRetain(image_buffer);
}

-static int vda_sync_decode(struct vda_context *vda_ctx)
+static int vda_sync_decode(VDAContext *ctx, struct vda_context *vda_ctx)
{
OSStatus status;
CFDataRef coded_frame;
uint32_t flush_flags = 1 << 0; ///< kVDADecoderFlush_emitFrames

coded_frame = CFDataCreate(kCFAllocatorDefault,
- vda_ctx->priv_bitstream,
- vda_ctx->priv_bitstream_size);
+ ctx->bitstream,
+ ctx->bitstream_size);

status = VDADecoderDecode(vda_ctx->decoder, 0, coded_frame, NULL);

@@ -71,12 +82,13 @@ static int vda_h264_start_frame(AVCodecContext *avctx,
av_unused const uint8_t *buffer,
av_unused uint32_t size)
{
+ VDAContext *vda = avctx->internal->hwaccel_priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;

if (!vda_ctx->decoder)
return -1;

- vda_ctx->priv_bitstream_size = 0;
+ vda->bitstream_size = 0;

return 0;
}
@@ -85,24 +97,25 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
const uint8_t *buffer,
uint32_t size)
{
+ VDAContext *vda = avctx->internal->hwaccel_priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
void *tmp;

if (!vda_ctx->decoder)
return -1;

- tmp = av_fast_realloc(vda_ctx->priv_bitstream,
- &vda_ctx->priv_allocated_size,
- vda_ctx->priv_bitstream_size + size + 4);
+ tmp = av_fast_realloc(vda->bitstream,
+ &vda->allocated_size,
+ vda->bitstream_size + size + 4);
if (!tmp)
return AVERROR(ENOMEM);

- vda_ctx->priv_bitstream = tmp;
+ vda->bitstream = tmp;

- AV_WB32(vda_ctx->priv_bitstream + vda_ctx->priv_bitstream_size, size);
- memcpy(vda_ctx->priv_bitstream + vda_ctx->priv_bitstream_size + 4, buffer, size);
+ AV_WB32(vda->bitstream + vda->bitstream_size, size);
+ memcpy(vda->bitstream + vda->bitstream_size + 4, buffer, size);

- vda_ctx->priv_bitstream_size += size + 4;
+ vda->bitstream_size += size + 4;

return 0;
}
@@ -110,14 +123,15 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
static int vda_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
+ VDAContext *vda = avctx->internal->hwaccel_priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
AVFrame *frame = &h->cur_pic_ptr->f;
int status;

- if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
+ if (!vda_ctx->decoder || !vda->bitstream)
return -1;

- status = vda_sync_decode(vda_ctx);
+ status = vda_sync_decode(vda, vda_ctx);
frame->data[3] = (void*)vda_ctx->cv_buffer;

if (status)
@@ -217,11 +231,15 @@ int ff_vda_destroy_decoder(struct vda_context *vda_ctx)
if (vda_ctx->decoder)
status = VDADecoderDestroy(vda_ctx->decoder);

- av_freep(&vda_ctx->priv_bitstream);
-
return status;
}

+static void vda_h264_uninit(AVCodecContext *avctx)
+{
+ VDAContext *vda = avctx->internal->priv_data;
+ av_freep(&vda->bitstream);
+}
+
AVHWAccel ff_h264_vda_hwaccel = {
.name = "h264_vda",
.type = AVMEDIA_TYPE_VIDEO,
@@ -230,4 +248,6 @@ AVHWAccel ff_h264_vda_hwaccel = {
.start_frame = vda_h264_start_frame,
.decode_slice = vda_h264_decode_slice,
.end_frame = vda_h264_end_frame,
+ .uninit = vda_h264_uninit,
+ .priv_data_size = sizeof(VDAContext),
};
--
1.8.5.2 (Apple Git-48)
Luca Barbato
2014-04-16 09:53:40 UTC
Permalink
On 24/03/14 03:01, Luca Barbato wrote:
> From: Anton Khirnov <***@khirnov.net>
>
> ---
> libavcodec/vda.h | 6 +++---
> libavcodec/vda_h264.c | 50 +++++++++++++++++++++++++++++++++++---------------
> 2 files changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/libavcodec/vda.h b/libavcodec/vda.h
> index 987b94f..1189e41 100644
> --- a/libavcodec/vda.h
> +++ b/libavcodec/vda.h
> @@ -112,17 +112,17 @@ struct vda_context {
> OSType cv_pix_fmt_type;
>
> /**
> - * The current bitstream buffer.
> + * unused
> */
> uint8_t *priv_bitstream;
>
> /**
> - * The current size of the bitstream.
> + * unused
> */
> int priv_bitstream_size;
>
> /**
> - * The reference size used for fast reallocation.
> + * unused
> */
> int priv_allocated_size;
> };
> diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
> index 6c1845a..72b0c78 100644
> --- a/libavcodec/vda_h264.c
> +++ b/libavcodec/vda_h264.c
> @@ -28,6 +28,17 @@
> #include "h264.h"
> #include "vda.h"
>
> +typedef struct VDAContext {
> + // The current bitstream buffer.
> + uint8_t *bitstream;
> +
> + // The current size of the bitstream.
> + int bitstream_size;
> +
> + // The reference size used for fast reallocation.
> + int allocated_size;
> +} VDAContext;
> +
> /* Decoder callback that adds the VDA frame to the queue in display order. */
> static void vda_decoder_callback(void *vda_hw_ctx,
> CFDictionaryRef user_info,
> @@ -46,15 +57,15 @@ static void vda_decoder_callback(void *vda_hw_ctx,
> vda_ctx->cv_buffer = CVPixelBufferRetain(image_buffer);
> }
>
> -static int vda_sync_decode(struct vda_context *vda_ctx)
> +static int vda_sync_decode(VDAContext *ctx, struct vda_context *vda_ctx)
> {
> OSStatus status;
> CFDataRef coded_frame;
> uint32_t flush_flags = 1 << 0; ///< kVDADecoderFlush_emitFrames
>
> coded_frame = CFDataCreate(kCFAllocatorDefault,
> - vda_ctx->priv_bitstream,
> - vda_ctx->priv_bitstream_size);
> + ctx->bitstream,
> + ctx->bitstream_size);
>
> status = VDADecoderDecode(vda_ctx->decoder, 0, coded_frame, NULL);
>
> @@ -71,12 +82,13 @@ static int vda_h264_start_frame(AVCodecContext *avctx,
> av_unused const uint8_t *buffer,
> av_unused uint32_t size)
> {
> + VDAContext *vda = avctx->internal->hwaccel_priv_data;
> struct vda_context *vda_ctx = avctx->hwaccel_context;
>
> if (!vda_ctx->decoder)
> return -1;
>
> - vda_ctx->priv_bitstream_size = 0;
> + vda->bitstream_size = 0;
>
> return 0;
> }
> @@ -85,24 +97,25 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
> const uint8_t *buffer,
> uint32_t size)
> {
> + VDAContext *vda = avctx->internal->hwaccel_priv_data;
> struct vda_context *vda_ctx = avctx->hwaccel_context;
> void *tmp;
>
> if (!vda_ctx->decoder)
> return -1;
>
> - tmp = av_fast_realloc(vda_ctx->priv_bitstream,
> - &vda_ctx->priv_allocated_size,
> - vda_ctx->priv_bitstream_size + size + 4);
> + tmp = av_fast_realloc(vda->bitstream,
> + &vda->allocated_size,
> + vda->bitstream_size + size + 4);
> if (!tmp)
> return AVERROR(ENOMEM);
>
> - vda_ctx->priv_bitstream = tmp;
> + vda->bitstream = tmp;
>
> - AV_WB32(vda_ctx->priv_bitstream + vda_ctx->priv_bitstream_size, size);
> - memcpy(vda_ctx->priv_bitstream + vda_ctx->priv_bitstream_size + 4, buffer, size);
> + AV_WB32(vda->bitstream + vda->bitstream_size, size);
> + memcpy(vda->bitstream + vda->bitstream_size + 4, buffer, size);
>
> - vda_ctx->priv_bitstream_size += size + 4;
> + vda->bitstream_size += size + 4;
>
> return 0;
> }
> @@ -110,14 +123,15 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
> static int vda_h264_end_frame(AVCodecContext *avctx)
> {
> H264Context *h = avctx->priv_data;
> + VDAContext *vda = avctx->internal->hwaccel_priv_data;
> struct vda_context *vda_ctx = avctx->hwaccel_context;
> AVFrame *frame = &h->cur_pic_ptr->f;
> int status;
>
> - if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
> + if (!vda_ctx->decoder || !vda->bitstream)
> return -1;
>
> - status = vda_sync_decode(vda_ctx);
> + status = vda_sync_decode(vda, vda_ctx);
> frame->data[3] = (void*)vda_ctx->cv_buffer;
>
> if (status)
> @@ -217,11 +231,15 @@ int ff_vda_destroy_decoder(struct vda_context *vda_ctx)
> if (vda_ctx->decoder)
> status = VDADecoderDestroy(vda_ctx->decoder);
>
> - av_freep(&vda_ctx->priv_bitstream);
> -
> return status;
> }
>
> +static void vda_h264_uninit(AVCodecContext *avctx)
> +{
> + VDAContext *vda = avctx->internal->priv_data;
> + av_freep(&vda->bitstream);
> +}
> +
> AVHWAccel ff_h264_vda_hwaccel = {
> .name = "h264_vda",
> .type = AVMEDIA_TYPE_VIDEO,
> @@ -230,4 +248,6 @@ AVHWAccel ff_h264_vda_hwaccel = {
> .start_frame = vda_h264_start_frame,
> .decode_slice = vda_h264_decode_slice,
> .end_frame = vda_h264_end_frame,
> + .uninit = vda_h264_uninit,
> + .priv_data_size = sizeof(VDAContext),
> };
>

Ping.
Luca Barbato
2014-03-24 02:01:53 UTC
Permalink
From: Anton Khirnov <***@khirnov.net>

It leverages the new hwaccel 1.2 features:

- get_buffer2 is never called
- the internal context is automatically initialized/deinitialized

Signed-off-by: Luca Barbato <***@gentoo.org>
---
libavcodec/Makefile | 1 +
libavcodec/allcodecs.c | 1 +
libavcodec/h264_slice.c | 23 ++++-
libavcodec/vda.c | 64 ++++++++++++
libavcodec/vda.h | 53 ++++++++++
libavcodec/vda_h264.c | 248 ++++++++++++++++++++++++++++++++++++++++++++--
libavcodec/vda_internal.h | 33 ++++++
7 files changed, 412 insertions(+), 11 deletions(-)
create mode 100644 libavcodec/vda.c
create mode 100644 libavcodec/vda_internal.h

diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 1b5a044..5908011 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -67,6 +67,7 @@ OBJS-$(CONFIG_RDFT) += rdft.o $(RDFT-OBJS-yes)
OBJS-$(CONFIG_SINEWIN) += sinewin.o
OBJS-$(CONFIG_TPELDSP) += tpeldsp.o
OBJS-$(CONFIG_VAAPI) += vaapi.o
+OBJS-$(CONFIG_VDA) += vda.o
OBJS-$(CONFIG_VDPAU) += vdpau.o
OBJS-$(CONFIG_VIDEODSP) += videodsp.o
OBJS-$(CONFIG_VP3DSP) += vp3dsp.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index ed6d7ff..7b061e6 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -79,6 +79,7 @@ void avcodec_register_all(void)
REGISTER_HWACCEL(H264_DXVA2, h264_dxva2);
REGISTER_HWACCEL(H264_VAAPI, h264_vaapi);
REGISTER_HWACCEL(H264_VDA, h264_vda);
+ REGISTER_HWACCEL(H264_VDA_OLD, h264_vda_old);
REGISTER_HWACCEL(H264_VDPAU, h264_vdpau);
REGISTER_HWACCEL(MPEG1_VDPAU, mpeg1_vdpau);
REGISTER_HWACCEL(MPEG2_DXVA2, mpeg2_dxva2);
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 73c0740..eb4adde 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -153,6 +153,7 @@ static const enum AVPixelFormat h264_hwaccel_pixfmt_list_420[] = {
#endif
#if CONFIG_H264_VDA_HWACCEL
AV_PIX_FMT_VDA_VLD,
+ AV_PIX_FMT_VDA,
#endif
#if CONFIG_H264_VDPAU_HWACCEL
AV_PIX_FMT_VDPAU,
@@ -170,6 +171,7 @@ static const enum AVPixelFormat h264_hwaccel_pixfmt_list_jpeg_420[] = {
#endif
#if CONFIG_H264_VDA_HWACCEL
AV_PIX_FMT_VDA_VLD,
+ AV_PIX_FMT_VDA,
#endif
#if CONFIG_H264_VDPAU_HWACCEL
AV_PIX_FMT_VDPAU,
@@ -247,10 +249,23 @@ static int alloc_picture(H264Context *h, H264Picture *pic)
av_assert0(!pic->f.data[0]);

pic->tf.f = &pic->f;
- ret = ff_thread_get_buffer(h->avctx, &pic->tf, pic->reference ?
- AV_GET_BUFFER_FLAG_REF : 0);
- if (ret < 0)
- goto fail;
+
+ if (h->avctx->pix_fmt == AV_PIX_FMT_VDA) {
+ /* the VDA framework handles its surfaces internally, so we
+ * do not call get_buffer at all;
+ * create a dummy buffer here until we get the real one later */
+ pic->f.buf[0] = av_buffer_alloc(1);
+ pic->f.width = h->avctx->width;
+ pic->f.height = h->avctx->height;
+ pic->f.format = h->avctx->pix_fmt;
+ if (!pic->f.buf[0])
+ goto fail;
+ } else {
+ ret = ff_thread_get_buffer(h->avctx, &pic->tf, pic->reference ?
+ AV_GET_BUFFER_FLAG_REF : 0);
+ if (ret < 0)
+ goto fail;
+ }

h->linesize = pic->f.linesize[0];
h->uvlinesize = pic->f.linesize[1];
diff --git a/libavcodec/vda.c b/libavcodec/vda.c
new file mode 100644
index 0000000..60f0b77
--- /dev/null
+++ b/libavcodec/vda.c
@@ -0,0 +1,64 @@
+/*
+ * This file is part of Libav.
+ *
+ * Libav is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * Libav is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with Libav; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "config.h"
+
+#include "libavutil/mem.h"
+
+#include "vda.h"
+#include "vda_internal.h"
+
+#if CONFIG_H264_VDA_HWACCEL
+AVVDAContext *av_vda_alloc_context(void)
+{
+ AVVDAContext *ret = av_mallocz(sizeof(*ret));
+
+ if (ret)
+ ret->output_callback = ff_vda_output_callback;
+
+ return ret;
+}
+
+int av_vda_default_init(AVCodecContext *avctx)
+{
+ avctx->hwaccel_context = av_vda_alloc_context();
+ if (!avctx->hwaccel_context)
+ return AVERROR(ENOMEM);
+ return ff_vda_default_init(avctx);
+}
+
+void av_vda_default_free(AVCodecContext *avctx)
+{
+ ff_vda_default_free(avctx);
+ av_freep(&avctx->hwaccel_context);
+}
+#else
+AVVDAContext *av_vda_alloc_context(void)
+{
+ return NULL;
+}
+
+int av_vda_default_init(AVCodecContext *avctx)
+{
+ return AVERROR(ENOSYS);
+}
+
+void av_vda_default_free(AVCodecContext *ctx)
+{
+}
+#endif
diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 1189e41..d9f935d 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -29,6 +29,7 @@
* Public libavcodec VDA header.
*/

+#include "libavcodec/avcodec.h"
#include "libavcodec/version.h"

#include <stdint.h>
@@ -136,6 +137,58 @@ int ff_vda_create_decoder(struct vda_context *vda_ctx,
int ff_vda_destroy_decoder(struct vda_context *vda_ctx);

/**
+ * This struct holds all the information that needs to be passed
+ * between the caller and libavcodec for initializing VDA decoding.
+ * Its size is not a part of the public ABI, it must be allocated with
+ * av_vda_alloc_context() and freed with av_free().
+ */
+typedef struct AVVDAContext {
+ /**
+ * VDA decoder object. Created and freed by the caller.
+ */
+ VDADecoder decoder;
+
+ /**
+ * The output callback that must be passed to VDADecoderCreate.
+ * Set by av_vda_alloc_context().
+ */
+ VDADecoderOutputCallback *output_callback;
+} AVVDAContext;
+
+/**
+ * Allocate and initialize a VDA context.
+ *
+ * This function should be called from the get_format() callback when the caller
+ * selects the AV_PIX_FMT_VDA format. The caller must then create the decoder
+ * object (using the output callback provided by libavcodec) that will be used
+ * for VDA-accelerated decoding.
+ *
+ * When decoding with VDA is finished, the caller must destroy the decoder
+ * object and free the VDA context using av_free().
+ *
+ * @return the newly allocated context or NULL on failure
+ */
+AVVDAContext *av_vda_alloc_context(void);
+
+/**
+ * This is a convenience function that creates and sets up the VDA context using
+ * an internal implementation.
+ *
+ * @param avctx the corresponding codec context
+ *
+ * @return >= 0 on success, a negative AVERROR code on failure
+ */
+int av_vda_default_init(AVCodecContext *avctx);
+
+/**
+ * This function must be called to free the VDA context initialized with
+ * av_vda_default_init().
+ *
+ * @param avctx the corresponding codec context
+ */
+void av_vda_default_free(AVCodecContext *avctx);
+
+/**
* @}
*/

diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 72b0c78..3cae992 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -26,7 +26,9 @@

#include "libavutil/avutil.h"
#include "h264.h"
+#include "internal.h"
#include "vda.h"
+#include "vda_internal.h"

typedef struct VDAContext {
// The current bitstream buffer.
@@ -37,6 +39,8 @@ typedef struct VDAContext {

// The reference size used for fast reallocation.
int allocated_size;
+
+ CVImageBufferRef frame;
} VDAContext;

/* Decoder callback that adds the VDA frame to the queue in display order. */
@@ -78,7 +82,7 @@ static int vda_sync_decode(VDAContext *ctx, struct vda_context *vda_ctx)
}


-static int vda_h264_start_frame(AVCodecContext *avctx,
+static int vda_old_h264_start_frame(AVCodecContext *avctx,
av_unused const uint8_t *buffer,
av_unused uint32_t size)
{
@@ -93,7 +97,7 @@ static int vda_h264_start_frame(AVCodecContext *avctx,
return 0;
}

-static int vda_h264_decode_slice(AVCodecContext *avctx,
+static int vda_old_h264_decode_slice(AVCodecContext *avctx,
const uint8_t *buffer,
uint32_t size)
{
@@ -120,7 +124,7 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
return 0;
}

-static int vda_h264_end_frame(AVCodecContext *avctx)
+static int vda_old_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
VDAContext *vda = avctx->internal->hwaccel_priv_data;
@@ -208,7 +212,7 @@ int ff_vda_create_decoder(struct vda_context *vda_ctx,

status = VDADecoderCreate(config_info,
buffer_attributes,
- vda_decoder_callback,
+ (VDADecoderOutputCallback *)vda_decoder_callback,
vda_ctx,
&vda_ctx->decoder);

@@ -234,17 +238,247 @@ int ff_vda_destroy_decoder(struct vda_context *vda_ctx)
return status;
}

-static void vda_h264_uninit(AVCodecContext *avctx)
+static int vda_h264_uninit(AVCodecContext *avctx)
{
- VDAContext *vda = avctx->internal->priv_data;
+ VDAContext *vda = avctx->internal->hwaccel_priv_data;
av_freep(&vda->bitstream);
+ if (vda->frame)
+ CVPixelBufferRelease(vda->frame);
+ return 0;
}

-AVHWAccel ff_h264_vda_hwaccel = {
+AVHWAccel ff_h264_vda_old_hwaccel = {
.name = "h264_vda",
.type = AVMEDIA_TYPE_VIDEO,
.id = AV_CODEC_ID_H264,
.pix_fmt = AV_PIX_FMT_VDA_VLD,
+ .start_frame = vda_old_h264_start_frame,
+ .decode_slice = vda_old_h264_decode_slice,
+ .end_frame = vda_old_h264_end_frame,
+ .uninit = vda_h264_uninit,
+ .priv_data_size = sizeof(VDAContext),
+};
+
+void ff_vda_output_callback(void *opaque,
+ CFDictionaryRef user_info,
+ OSStatus status,
+ uint32_t infoFlags,
+ CVImageBufferRef image_buffer)
+{
+ AVCodecContext *ctx = opaque;
+ VDAContext *vda = ctx->internal->hwaccel_priv_data;
+
+
+ if (vda->frame) {
+ CVPixelBufferRelease(vda->frame);
+ vda->frame = NULL;
+ }
+
+ if (!image_buffer)
+ return;
+
+ vda->frame = CVPixelBufferRetain(image_buffer);
+}
+
+static int vda_h264_start_frame(AVCodecContext *avctx,
+ const uint8_t *buffer,
+ uint32_t size)
+{
+ VDAContext *vda = avctx->internal->hwaccel_priv_data;
+
+ vda->bitstream_size = 0;
+
+ return 0;
+}
+
+static int vda_h264_decode_slice(AVCodecContext *avctx,
+ const uint8_t *buffer,
+ uint32_t size)
+{
+ VDAContext *vda = avctx->internal->hwaccel_priv_data;
+ void *tmp;
+
+ tmp = av_fast_realloc(vda->bitstream,
+ &vda->allocated_size,
+ vda->bitstream_size + size + 4);
+ if (!tmp)
+ return AVERROR(ENOMEM);
+
+ vda->bitstream = tmp;
+
+ AV_WB32(vda->bitstream + vda->bitstream_size, size);
+ memcpy(vda->bitstream + vda->bitstream_size + 4, buffer, size);
+
+ vda->bitstream_size += size + 4;
+
+ return 0;
+}
+
+static void release_buffer(void *opaque, uint8_t *data)
+{
+ CVImageBufferRef frame = (CVImageBufferRef)data;
+ CVPixelBufferRelease(frame);
+}
+
+static int vda_h264_end_frame(AVCodecContext *avctx)
+{
+ H264Context *h = avctx->priv_data;
+ VDAContext *vda = avctx->internal->hwaccel_priv_data;
+ AVVDAContext *vda_ctx = avctx->hwaccel_context;
+ AVFrame *frame = &h->cur_pic_ptr->f;
+ uint32_t flush_flags = 1 << 0; ///< kVDADecoderFlush_emitFrames
+ CFDataRef coded_frame;
+ OSStatus status;
+
+ if (!vda->bitstream_size)
+ return AVERROR_INVALIDDATA;
+
+
+ coded_frame = CFDataCreate(kCFAllocatorDefault,
+ vda->bitstream,
+ vda->bitstream_size);
+
+ status = VDADecoderDecode(vda_ctx->decoder, 0, coded_frame, NULL);
+
+ if (status == kVDADecoderNoErr)
+ status = VDADecoderFlush(vda_ctx->decoder, flush_flags);
+
+ CFRelease(coded_frame);
+
+ if (status != kVDADecoderNoErr) {
+ av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);
+ return AVERROR_UNKNOWN;
+ }
+
+ if (vda->frame) {
+ av_buffer_unref(&frame->buf[0]);
+
+ frame->buf[0] = av_buffer_create((uint8_t*)vda->frame,
+ sizeof(vda->frame),
+ release_buffer, NULL,
+ AV_BUFFER_FLAG_READONLY);
+ if (!frame->buf)
+ return AVERROR(ENOMEM);
+
+ frame->data[3] = (uint8_t*)vda->frame;
+ vda->frame = NULL;
+ }
+
+ return 0;
+}
+
+int ff_vda_default_init(AVCodecContext *avctx)
+{
+ AVVDAContext *vda_ctx = avctx->hwaccel_context;
+ OSStatus status = kVDADecoderNoErr;
+ CFNumberRef height;
+ CFNumberRef width;
+ CFNumberRef format;
+ CFDataRef avc_data;
+ CFMutableDictionaryRef config_info;
+ CFMutableDictionaryRef buffer_attributes;
+ CFMutableDictionaryRef io_surface_properties;
+ CFNumberRef cv_pix_fmt;
+ int32_t fmt = 'avc1', pix_fmt = kCVPixelFormatType_422YpCbCr8;
+
+ // kCVPixelFormatType_420YpCbCr8Planar;
+
+ /* Each VCL NAL in the bistream sent to the decoder
+ * is preceded by a 4 bytes length header.
+ * Change the avcC atom header if needed, to signal headers of 4 bytes. */
+ if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 0x03) {
+ uint8_t *rw_extradata;
+
+ if (!(rw_extradata = av_malloc(avctx->extradata_size)))
+ return AVERROR(ENOMEM);
+
+ memcpy(rw_extradata, avctx->extradata, avctx->extradata_size);
+
+ rw_extradata[4] |= 0x03;
+
+ avc_data = CFDataCreate(kCFAllocatorDefault, rw_extradata, avctx->extradata_size);
+
+ av_freep(&rw_extradata);
+ } else {
+ avc_data = CFDataCreate(kCFAllocatorDefault,
+ avctx->extradata, avctx->extradata_size);
+ }
+
+ config_info = CFDictionaryCreateMutable(kCFAllocatorDefault,
+ 4,
+ &kCFTypeDictionaryKeyCallBacks,
+ &kCFTypeDictionaryValueCallBacks);
+
+ height = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &avctx->height);
+ width = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &avctx->width);
+ format = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &fmt);
+ CFDictionarySetValue(config_info, kVDADecoderConfiguration_Height, height);
+ CFDictionarySetValue(config_info, kVDADecoderConfiguration_Width, width);
+ CFDictionarySetValue(config_info, kVDADecoderConfiguration_avcCData, avc_data);
+ CFDictionarySetValue(config_info, kVDADecoderConfiguration_SourceFormat, format);
+
+ buffer_attributes = CFDictionaryCreateMutable(kCFAllocatorDefault,
+ 2,
+ &kCFTypeDictionaryKeyCallBacks,
+ &kCFTypeDictionaryValueCallBacks);
+ io_surface_properties = CFDictionaryCreateMutable(kCFAllocatorDefault,
+ 0,
+ &kCFTypeDictionaryKeyCallBacks,
+ &kCFTypeDictionaryValueCallBacks);
+ cv_pix_fmt = CFNumberCreate(kCFAllocatorDefault,
+ kCFNumberSInt32Type,
+ &pix_fmt);
+
+ CFDictionarySetValue(buffer_attributes,
+ kCVPixelBufferPixelFormatTypeKey,
+ cv_pix_fmt);
+ CFDictionarySetValue(buffer_attributes,
+ kCVPixelBufferIOSurfacePropertiesKey,
+ io_surface_properties);
+
+ status = VDADecoderCreate(config_info,
+ buffer_attributes,
+ (VDADecoderOutputCallback *)ff_vda_output_callback,
+ avctx,
+ &vda_ctx->decoder);
+
+ CFRelease(format);
+ CFRelease(height);
+ CFRelease(width);
+ CFRelease(avc_data);
+ CFRelease(config_info);
+
+ if (status != kVDADecoderNoErr) {
+ av_log(avctx, AV_LOG_ERROR, "Cannot initialize VDA %d\n", status);
+ }
+
+ switch (status) {
+ case kVDADecoderHardwareNotSupportedErr:
+ case kVDADecoderFormatNotSupportedErr:
+ return AVERROR(ENOSYS);
+ case kVDADecoderConfigurationError:
+ return AVERROR(EINVAL);
+ case kVDADecoderDecoderFailedErr:
+ return AVERROR_INVALIDDATA;
+ case kVDADecoderNoErr:
+ return 0;
+ default:
+ return AVERROR_UNKNOWN;
+ }
+}
+
+void ff_vda_default_free(AVCodecContext *avctx)
+{
+ AVVDAContext *vda = avctx->hwaccel_context;
+ if (vda && vda->decoder)
+ VDADecoderDestroy(vda->decoder);
+}
+
+AVHWAccel ff_h264_vda_hwaccel = {
+ .name = "h264_vda",
+ .type = AVMEDIA_TYPE_VIDEO,
+ .id = AV_CODEC_ID_H264,
+ .pix_fmt = AV_PIX_FMT_VDA,
.start_frame = vda_h264_start_frame,
.decode_slice = vda_h264_decode_slice,
.end_frame = vda_h264_end_frame,
diff --git a/libavcodec/vda_internal.h b/libavcodec/vda_internal.h
new file mode 100644
index 0000000..9d0ed80
--- /dev/null
+++ b/libavcodec/vda_internal.h
@@ -0,0 +1,33 @@
+/*
+ * This file is part of Libav.
+ *
+ * Libav is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * Libav is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with Libav; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef AVCODEC_VDA_INTERNAL_H
+#define AVCODEC_VDA_INTERNAL_H
+
+#include "vda.h"
+
+void ff_vda_output_callback(void *vda_hw_ctx,
+ CFDictionaryRef user_info,
+ OSStatus status,
+ uint32_t infoFlags,
+ CVImageBufferRef image_buffer);
+
+int ff_vda_default_init(AVCodecContext *avctx);
+void ff_vda_default_free(AVCodecContext *avctx);
+
+#endif /* AVCODEC_VDA_INTERNAL_H */
--
1.8.5.2 (Apple Git-48)
Luca Barbato
2014-03-24 17:38:50 UTC
Permalink
On 24/03/14 03:01, Luca Barbato wrote:

> + CFRelease(format);
> + CFRelease(height);
> + CFRelease(width);
> + CFRelease(avc_data);
> + CFRelease(config_info);

Added locally the other release required.

lu
wm4
2014-03-24 20:02:17 UTC
Permalink
On Mon, 24 Mar 2014 03:01:53 +0100
Luca Barbato <***@gentoo.org> wrote:

> From: Anton Khirnov <***@khirnov.net>
>
> It leverages the new hwaccel 1.2 features:
>
> - get_buffer2 is never called
> - the internal context is automatically initialized/deinitialized
>
> Signed-off-by: Luca Barbato <***@gentoo.org>
> ---
> libavcodec/Makefile | 1 +
> libavcodec/allcodecs.c | 1 +
> libavcodec/h264_slice.c | 23 ++++-
> libavcodec/vda.c | 64 ++++++++++++
> libavcodec/vda.h | 53 ++++++++++
> libavcodec/vda_h264.c | 248 ++++++++++++++++++++++++++++++++++++++++++++--
> libavcodec/vda_internal.h | 33 ++++++
> 7 files changed, 412 insertions(+), 11 deletions(-)
> create mode 100644 libavcodec/vda.c
> create mode 100644 libavcodec/vda_internal.h
>
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 1b5a044..5908011 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -67,6 +67,7 @@ OBJS-$(CONFIG_RDFT) += rdft.o $(RDFT-OBJS-yes)
> OBJS-$(CONFIG_SINEWIN) += sinewin.o
> OBJS-$(CONFIG_TPELDSP) += tpeldsp.o
> OBJS-$(CONFIG_VAAPI) += vaapi.o
> +OBJS-$(CONFIG_VDA) += vda.o
> OBJS-$(CONFIG_VDPAU) += vdpau.o
> OBJS-$(CONFIG_VIDEODSP) += videodsp.o
> OBJS-$(CONFIG_VP3DSP) += vp3dsp.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index ed6d7ff..7b061e6 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -79,6 +79,7 @@ void avcodec_register_all(void)
> REGISTER_HWACCEL(H264_DXVA2, h264_dxva2);
> REGISTER_HWACCEL(H264_VAAPI, h264_vaapi);
> REGISTER_HWACCEL(H264_VDA, h264_vda);
> + REGISTER_HWACCEL(H264_VDA_OLD, h264_vda_old);
> REGISTER_HWACCEL(H264_VDPAU, h264_vdpau);
> REGISTER_HWACCEL(MPEG1_VDPAU, mpeg1_vdpau);
> REGISTER_HWACCEL(MPEG2_DXVA2, mpeg2_dxva2);
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 73c0740..eb4adde 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -153,6 +153,7 @@ static const enum AVPixelFormat h264_hwaccel_pixfmt_list_420[] = {
> #endif
> #if CONFIG_H264_VDA_HWACCEL
> AV_PIX_FMT_VDA_VLD,
> + AV_PIX_FMT_VDA,
> #endif
> #if CONFIG_H264_VDPAU_HWACCEL
> AV_PIX_FMT_VDPAU,
> @@ -170,6 +171,7 @@ static const enum AVPixelFormat h264_hwaccel_pixfmt_list_jpeg_420[] = {
> #endif
> #if CONFIG_H264_VDA_HWACCEL
> AV_PIX_FMT_VDA_VLD,
> + AV_PIX_FMT_VDA,
> #endif
> #if CONFIG_H264_VDPAU_HWACCEL
> AV_PIX_FMT_VDPAU,
> @@ -247,10 +249,23 @@ static int alloc_picture(H264Context *h, H264Picture *pic)
> av_assert0(!pic->f.data[0]);
>
> pic->tf.f = &pic->f;
> - ret = ff_thread_get_buffer(h->avctx, &pic->tf, pic->reference ?
> - AV_GET_BUFFER_FLAG_REF : 0);
> - if (ret < 0)
> - goto fail;
> +
> + if (h->avctx->pix_fmt == AV_PIX_FMT_VDA) {
> + /* the VDA framework handles its surfaces internally, so we
> + * do not call get_buffer at all;
> + * create a dummy buffer here until we get the real one later */
> + pic->f.buf[0] = av_buffer_alloc(1);
> + pic->f.width = h->avctx->width;
> + pic->f.height = h->avctx->height;
> + pic->f.format = h->avctx->pix_fmt;
> + if (!pic->f.buf[0])
> + goto fail;
> + } else {
> + ret = ff_thread_get_buffer(h->avctx, &pic->tf, pic->reference ?
> + AV_GET_BUFFER_FLAG_REF : 0);
> + if (ret < 0)
> + goto fail;
> + }

Maybe this could be made a bit nicer by adding a new callback to
AVHWAccel? If that callback is NULL, the code would fall back to
ff_thread_get_buffer.

> h->linesize = pic->f.linesize[0];
> h->uvlinesize = pic->f.linesize[1];
> diff --git a/libavcodec/vda.c b/libavcodec/vda.c
> new file mode 100644
> index 0000000..60f0b77
> --- /dev/null
> +++ b/libavcodec/vda.c
> @@ -0,0 +1,64 @@
> +/*
> + * This file is part of Libav.
> + *
> + * Libav is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * Libav is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with Libav; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "config.h"
> +
> +#include "libavutil/mem.h"
> +
> +#include "vda.h"
> +#include "vda_internal.h"
> +
> +#if CONFIG_H264_VDA_HWACCEL
> +AVVDAContext *av_vda_alloc_context(void)
> +{
> + AVVDAContext *ret = av_mallocz(sizeof(*ret));
> +
> + if (ret)
> + ret->output_callback = ff_vda_output_callback;
> +
> + return ret;
> +}
> +
> +int av_vda_default_init(AVCodecContext *avctx)
> +{
> + avctx->hwaccel_context = av_vda_alloc_context();
> + if (!avctx->hwaccel_context)
> + return AVERROR(ENOMEM);
> + return ff_vda_default_init(avctx);
> +}
> +
> +void av_vda_default_free(AVCodecContext *avctx)
> +{
> + ff_vda_default_free(avctx);
> + av_freep(&avctx->hwaccel_context);
> +}
> +#else
> +AVVDAContext *av_vda_alloc_context(void)
> +{
> + return NULL;
> +}
> +
> +int av_vda_default_init(AVCodecContext *avctx)
> +{
> + return AVERROR(ENOSYS);
> +}
> +
> +void av_vda_default_free(AVCodecContext *ctx)
> +{
> +}
> +#endif
> diff --git a/libavcodec/vda.h b/libavcodec/vda.h
> index 1189e41..d9f935d 100644
> --- a/libavcodec/vda.h
> +++ b/libavcodec/vda.h
> @@ -29,6 +29,7 @@
> * Public libavcodec VDA header.
> */
>
> +#include "libavcodec/avcodec.h"
> #include "libavcodec/version.h"
>
> #include <stdint.h>
> @@ -136,6 +137,58 @@ int ff_vda_create_decoder(struct vda_context *vda_ctx,
> int ff_vda_destroy_decoder(struct vda_context *vda_ctx);
>
> /**
> + * This struct holds all the information that needs to be passed
> + * between the caller and libavcodec for initializing VDA decoding.
> + * Its size is not a part of the public ABI, it must be allocated with
> + * av_vda_alloc_context() and freed with av_free().
> + */
> +typedef struct AVVDAContext {
> + /**
> + * VDA decoder object. Created and freed by the caller.
> + */
> + VDADecoder decoder;
> +
> + /**
> + * The output callback that must be passed to VDADecoderCreate.
> + * Set by av_vda_alloc_context().
> + */
> + VDADecoderOutputCallback *output_callback;
> +} AVVDAContext;
> +
> +/**
> + * Allocate and initialize a VDA context.
> + *
> + * This function should be called from the get_format() callback when the caller
> + * selects the AV_PIX_FMT_VDA format. The caller must then create the decoder
> + * object (using the output callback provided by libavcodec) that will be used
> + * for VDA-accelerated decoding.
> + *
> + * When decoding with VDA is finished, the caller must destroy the decoder
> + * object and free the VDA context using av_free().
> + *
> + * @return the newly allocated context or NULL on failure
> + */
> +AVVDAContext *av_vda_alloc_context(void);

I wonder why this is needed at all. Couldn't the hwaccel allocate this
on its own? On the other hand, maybe it's better to keep this function,
to make interaction with get_format easier.

(I'd argue there should be explicit init and uninit callbacks, rather
than just get_format. Currently, it's somewhat hard to tell when you
should allocate the hwaccel context and when you should free it.
Especially with scary use cases in mind like switching hwaccels
mid-stream, which is supposed to work.)

> +/**
> + * This is a convenience function that creates and sets up the VDA context using
> + * an internal implementation.
> + *
> + * @param avctx the corresponding codec context
> + *
> + * @return >= 0 on success, a negative AVERROR code on failure
> + */
> +int av_vda_default_init(AVCodecContext *avctx);
> +
> +/**
> + * This function must be called to free the VDA context initialized with
> + * av_vda_default_init().
> + *
> + * @param avctx the corresponding codec context
> + */
> +void av_vda_default_free(AVCodecContext *avctx);
> +
> +/**
> * @}
> */
>
> diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
> index 72b0c78..3cae992 100644
> --- a/libavcodec/vda_h264.c
> +++ b/libavcodec/vda_h264.c
> @@ -26,7 +26,9 @@
>
> #include "libavutil/avutil.h"
> #include "h264.h"
> +#include "internal.h"
> #include "vda.h"
> +#include "vda_internal.h"
>
> typedef struct VDAContext {
> // The current bitstream buffer.
> @@ -37,6 +39,8 @@ typedef struct VDAContext {
>
> // The reference size used for fast reallocation.
> int allocated_size;
> +
> + CVImageBufferRef frame;
> } VDAContext;
>
> /* Decoder callback that adds the VDA frame to the queue in display order. */
> @@ -78,7 +82,7 @@ static int vda_sync_decode(VDAContext *ctx, struct vda_context *vda_ctx)
> }
>
>
> -static int vda_h264_start_frame(AVCodecContext *avctx,
> +static int vda_old_h264_start_frame(AVCodecContext *avctx,
> av_unused const uint8_t *buffer,
> av_unused uint32_t size)
> {
> @@ -93,7 +97,7 @@ static int vda_h264_start_frame(AVCodecContext *avctx,
> return 0;
> }
>
> -static int vda_h264_decode_slice(AVCodecContext *avctx,
> +static int vda_old_h264_decode_slice(AVCodecContext *avctx,
> const uint8_t *buffer,
> uint32_t size)
> {
> @@ -120,7 +124,7 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
> return 0;
> }
>
> -static int vda_h264_end_frame(AVCodecContext *avctx)
> +static int vda_old_h264_end_frame(AVCodecContext *avctx)
> {
> H264Context *h = avctx->priv_data;
> VDAContext *vda = avctx->internal->hwaccel_priv_data;
> @@ -208,7 +212,7 @@ int ff_vda_create_decoder(struct vda_context *vda_ctx,
>
> status = VDADecoderCreate(config_info,
> buffer_attributes,
> - vda_decoder_callback,
> + (VDADecoderOutputCallback *)vda_decoder_callback,
> vda_ctx,
> &vda_ctx->decoder);
>
> @@ -234,17 +238,247 @@ int ff_vda_destroy_decoder(struct vda_context *vda_ctx)
> return status;
> }
>
> -static void vda_h264_uninit(AVCodecContext *avctx)
> +static int vda_h264_uninit(AVCodecContext *avctx)
> {
> - VDAContext *vda = avctx->internal->priv_data;
> + VDAContext *vda = avctx->internal->hwaccel_priv_data;
> av_freep(&vda->bitstream);
> + if (vda->frame)
> + CVPixelBufferRelease(vda->frame);
> + return 0;
> }
>
> -AVHWAccel ff_h264_vda_hwaccel = {
> +AVHWAccel ff_h264_vda_old_hwaccel = {
> .name = "h264_vda",
> .type = AVMEDIA_TYPE_VIDEO,
> .id = AV_CODEC_ID_H264,
> .pix_fmt = AV_PIX_FMT_VDA_VLD,
> + .start_frame = vda_old_h264_start_frame,
> + .decode_slice = vda_old_h264_decode_slice,
> + .end_frame = vda_old_h264_end_frame,
> + .uninit = vda_h264_uninit,
> + .priv_data_size = sizeof(VDAContext),
> +};
> +
> +void ff_vda_output_callback(void *opaque,
> + CFDictionaryRef user_info,
> + OSStatus status,
> + uint32_t infoFlags,
> + CVImageBufferRef image_buffer)
> +{
> + AVCodecContext *ctx = opaque;
> + VDAContext *vda = ctx->internal->hwaccel_priv_data;
> +
> +
> + if (vda->frame) {
> + CVPixelBufferRelease(vda->frame);
> + vda->frame = NULL;
> + }
> +
> + if (!image_buffer)
> + return;
> +
> + vda->frame = CVPixelBufferRetain(image_buffer);
> +}
> +
> +static int vda_h264_start_frame(AVCodecContext *avctx,
> + const uint8_t *buffer,
> + uint32_t size)
> +{
> + VDAContext *vda = avctx->internal->hwaccel_priv_data;
> +
> + vda->bitstream_size = 0;
> +
> + return 0;
> +}
> +
> +static int vda_h264_decode_slice(AVCodecContext *avctx,
> + const uint8_t *buffer,
> + uint32_t size)
> +{
> + VDAContext *vda = avctx->internal->hwaccel_priv_data;
> + void *tmp;
> +
> + tmp = av_fast_realloc(vda->bitstream,
> + &vda->allocated_size,
> + vda->bitstream_size + size + 4);
> + if (!tmp)
> + return AVERROR(ENOMEM);
> +
> + vda->bitstream = tmp;
> +
> + AV_WB32(vda->bitstream + vda->bitstream_size, size);
> + memcpy(vda->bitstream + vda->bitstream_size + 4, buffer, size);
> +
> + vda->bitstream_size += size + 4;
> +
> + return 0;
> +}
> +
> +static void release_buffer(void *opaque, uint8_t *data)
> +{
> + CVImageBufferRef frame = (CVImageBufferRef)data;
> + CVPixelBufferRelease(frame);
> +}
> +
> +static int vda_h264_end_frame(AVCodecContext *avctx)
> +{
> + H264Context *h = avctx->priv_data;
> + VDAContext *vda = avctx->internal->hwaccel_priv_data;
> + AVVDAContext *vda_ctx = avctx->hwaccel_context;
> + AVFrame *frame = &h->cur_pic_ptr->f;
> + uint32_t flush_flags = 1 << 0; ///< kVDADecoderFlush_emitFrames
> + CFDataRef coded_frame;
> + OSStatus status;
> +
> + if (!vda->bitstream_size)
> + return AVERROR_INVALIDDATA;
> +
> +
> + coded_frame = CFDataCreate(kCFAllocatorDefault,
> + vda->bitstream,
> + vda->bitstream_size);
> +
> + status = VDADecoderDecode(vda_ctx->decoder, 0, coded_frame, NULL);
> +
> + if (status == kVDADecoderNoErr)
> + status = VDADecoderFlush(vda_ctx->decoder, flush_flags);
> +
> + CFRelease(coded_frame);
> +
> + if (status != kVDADecoderNoErr) {
> + av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);
> + return AVERROR_UNKNOWN;
> + }
> +
> + if (vda->frame) {
> + av_buffer_unref(&frame->buf[0]);
> +
> + frame->buf[0] = av_buffer_create((uint8_t*)vda->frame,
> + sizeof(vda->frame),
> + release_buffer, NULL,
> + AV_BUFFER_FLAG_READONLY);
> + if (!frame->buf)
> + return AVERROR(ENOMEM);
> +
> + frame->data[3] = (uint8_t*)vda->frame;
> + vda->frame = NULL;
> + }
> +
> + return 0;
> +}
> +
> +int ff_vda_default_init(AVCodecContext *avctx)

Maybe I'm missing something here, but shouldn't this be
av_vda_default_init?

> +{
> + AVVDAContext *vda_ctx = avctx->hwaccel_context;
> + OSStatus status = kVDADecoderNoErr;
> + CFNumberRef height;
> + CFNumberRef width;
> + CFNumberRef format;
> + CFDataRef avc_data;
> + CFMutableDictionaryRef config_info;
> + CFMutableDictionaryRef buffer_attributes;
> + CFMutableDictionaryRef io_surface_properties;
> + CFNumberRef cv_pix_fmt;
> + int32_t fmt = 'avc1', pix_fmt = kCVPixelFormatType_422YpCbCr8;
> +
> + // kCVPixelFormatType_420YpCbCr8Planar;
> +
> + /* Each VCL NAL in the bistream sent to the decoder
> + * is preceded by a 4 bytes length header.
> + * Change the avcC atom header if needed, to signal headers of 4 bytes. */
> + if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 0x03) {
> + uint8_t *rw_extradata;
> +
> + if (!(rw_extradata = av_malloc(avctx->extradata_size)))
> + return AVERROR(ENOMEM);
> +
> + memcpy(rw_extradata, avctx->extradata, avctx->extradata_size);
> +
> + rw_extradata[4] |= 0x03;
> +
> + avc_data = CFDataCreate(kCFAllocatorDefault, rw_extradata, avctx->extradata_size);
> +
> + av_freep(&rw_extradata);
> + } else {
> + avc_data = CFDataCreate(kCFAllocatorDefault,
> + avctx->extradata, avctx->extradata_size);
> + }
> +
> + config_info = CFDictionaryCreateMutable(kCFAllocatorDefault,
> + 4,
> + &kCFTypeDictionaryKeyCallBacks,
> + &kCFTypeDictionaryValueCallBacks);
> +
> + height = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &avctx->height);
> + width = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &avctx->width);
> + format = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &fmt);
> + CFDictionarySetValue(config_info, kVDADecoderConfiguration_Height, height);
> + CFDictionarySetValue(config_info, kVDADecoderConfiguration_Width, width);
> + CFDictionarySetValue(config_info, kVDADecoderConfiguration_avcCData, avc_data);
> + CFDictionarySetValue(config_info, kVDADecoderConfiguration_SourceFormat, format);
> +
> + buffer_attributes = CFDictionaryCreateMutable(kCFAllocatorDefault,
> + 2,
> + &kCFTypeDictionaryKeyCallBacks,
> + &kCFTypeDictionaryValueCallBacks);
> + io_surface_properties = CFDictionaryCreateMutable(kCFAllocatorDefault,
> + 0,
> + &kCFTypeDictionaryKeyCallBacks,
> + &kCFTypeDictionaryValueCallBacks);
> + cv_pix_fmt = CFNumberCreate(kCFAllocatorDefault,
> + kCFNumberSInt32Type,
> + &pix_fmt);
> +
> + CFDictionarySetValue(buffer_attributes,
> + kCVPixelBufferPixelFormatTypeKey,
> + cv_pix_fmt);
> + CFDictionarySetValue(buffer_attributes,
> + kCVPixelBufferIOSurfacePropertiesKey,
> + io_surface_properties);
> +
> + status = VDADecoderCreate(config_info,
> + buffer_attributes,
> + (VDADecoderOutputCallback *)ff_vda_output_callback,
> + avctx,
> + &vda_ctx->decoder);
> +
> + CFRelease(format);
> + CFRelease(height);
> + CFRelease(width);
> + CFRelease(avc_data);
> + CFRelease(config_info);
> +
> + if (status != kVDADecoderNoErr) {
> + av_log(avctx, AV_LOG_ERROR, "Cannot initialize VDA %d\n", status);
> + }
> +
> + switch (status) {
> + case kVDADecoderHardwareNotSupportedErr:
> + case kVDADecoderFormatNotSupportedErr:
> + return AVERROR(ENOSYS);
> + case kVDADecoderConfigurationError:
> + return AVERROR(EINVAL);
> + case kVDADecoderDecoderFailedErr:
> + return AVERROR_INVALIDDATA;

I'm really quite suspicious about this error code mapping. API users
need the guarantee that this mapping if fixed and that there's e.g. no
other reason AVERROR_INVALIDDATA could be returned. Or even better:
return the native VDA error code in some way. Either directly, or using
an additional parameter?

> + case kVDADecoderNoErr:
> + return 0;
> + default:
> + return AVERROR_UNKNOWN;
> + }
> +}
> +
> +void ff_vda_default_free(AVCodecContext *avctx)
> +{
> + AVVDAContext *vda = avctx->hwaccel_context;
> + if (vda && vda->decoder)
> + VDADecoderDestroy(vda->decoder);
> +}
> +
> +AVHWAccel ff_h264_vda_hwaccel = {
> + .name = "h264_vda",
> + .type = AVMEDIA_TYPE_VIDEO,
> + .id = AV_CODEC_ID_H264,
> + .pix_fmt = AV_PIX_FMT_VDA,
> .start_frame = vda_h264_start_frame,
> .decode_slice = vda_h264_decode_slice,
> .end_frame = vda_h264_end_frame,
> diff --git a/libavcodec/vda_internal.h b/libavcodec/vda_internal.h
> new file mode 100644
> index 0000000..9d0ed80
> --- /dev/null
> +++ b/libavcodec/vda_internal.h
> @@ -0,0 +1,33 @@
> +/*
> + * This file is part of Libav.
> + *
> + * Libav is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * Libav is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with Libav; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVCODEC_VDA_INTERNAL_H
> +#define AVCODEC_VDA_INTERNAL_H
> +
> +#include "vda.h"
> +
> +void ff_vda_output_callback(void *vda_hw_ctx,
> + CFDictionaryRef user_info,
> + OSStatus status,
> + uint32_t infoFlags,
> + CVImageBufferRef image_buffer);
> +
> +int ff_vda_default_init(AVCodecContext *avctx);
> +void ff_vda_default_free(AVCodecContext *avctx);
> +
> +#endif /* AVCODEC_VDA_INTERNAL_H */
Luca Barbato
2014-03-24 20:19:35 UTC
Permalink
On 24/03/14 21:02, wm4 wrote:
> On Mon, 24 Mar 2014 03:01:53 +0100
> Luca Barbato <***@gentoo.org> wrote:
>
>> From: Anton Khirnov <***@khirnov.net>
>>
>> It leverages the new hwaccel 1.2 features:
>>
>> - get_buffer2 is never called
>> - the internal context is automatically initialized/deinitialized
>>
>> Signed-off-by: Luca Barbato <***@gentoo.org>
>> ---
>> libavcodec/Makefile | 1 +
>> libavcodec/allcodecs.c | 1 +
>> libavcodec/h264_slice.c | 23 ++++-
>> libavcodec/vda.c | 64 ++++++++++++
>> libavcodec/vda.h | 53 ++++++++++
>> libavcodec/vda_h264.c | 248 ++++++++++++++++++++++++++++++++++++++++++++--
>> libavcodec/vda_internal.h | 33 ++++++
>> 7 files changed, 412 insertions(+), 11 deletions(-)
>> create mode 100644 libavcodec/vda.c
>> create mode 100644 libavcodec/vda_internal.h
>>
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index 1b5a044..5908011 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -67,6 +67,7 @@ OBJS-$(CONFIG_RDFT) += rdft.o $(RDFT-OBJS-yes)
>> OBJS-$(CONFIG_SINEWIN) += sinewin.o
>> OBJS-$(CONFIG_TPELDSP) += tpeldsp.o
>> OBJS-$(CONFIG_VAAPI) += vaapi.o
>> +OBJS-$(CONFIG_VDA) += vda.o
>> OBJS-$(CONFIG_VDPAU) += vdpau.o
>> OBJS-$(CONFIG_VIDEODSP) += videodsp.o
>> OBJS-$(CONFIG_VP3DSP) += vp3dsp.o
>> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
>> index ed6d7ff..7b061e6 100644
>> --- a/libavcodec/allcodecs.c
>> +++ b/libavcodec/allcodecs.c
>> @@ -79,6 +79,7 @@ void avcodec_register_all(void)
>> REGISTER_HWACCEL(H264_DXVA2, h264_dxva2);
>> REGISTER_HWACCEL(H264_VAAPI, h264_vaapi);
>> REGISTER_HWACCEL(H264_VDA, h264_vda);
>> + REGISTER_HWACCEL(H264_VDA_OLD, h264_vda_old);
>> REGISTER_HWACCEL(H264_VDPAU, h264_vdpau);
>> REGISTER_HWACCEL(MPEG1_VDPAU, mpeg1_vdpau);
>> REGISTER_HWACCEL(MPEG2_DXVA2, mpeg2_dxva2);
>> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
>> index 73c0740..eb4adde 100644
>> --- a/libavcodec/h264_slice.c
>> +++ b/libavcodec/h264_slice.c
>> @@ -153,6 +153,7 @@ static const enum AVPixelFormat h264_hwaccel_pixfmt_list_420[] = {
>> #endif
>> #if CONFIG_H264_VDA_HWACCEL
>> AV_PIX_FMT_VDA_VLD,
>> + AV_PIX_FMT_VDA,
>> #endif
>> #if CONFIG_H264_VDPAU_HWACCEL
>> AV_PIX_FMT_VDPAU,
>> @@ -170,6 +171,7 @@ static const enum AVPixelFormat h264_hwaccel_pixfmt_list_jpeg_420[] = {
>> #endif
>> #if CONFIG_H264_VDA_HWACCEL
>> AV_PIX_FMT_VDA_VLD,
>> + AV_PIX_FMT_VDA,
>> #endif
>> #if CONFIG_H264_VDPAU_HWACCEL
>> AV_PIX_FMT_VDPAU,
>> @@ -247,10 +249,23 @@ static int alloc_picture(H264Context *h, H264Picture *pic)
>> av_assert0(!pic->f.data[0]);
>>
>> pic->tf.f = &pic->f;
>> - ret = ff_thread_get_buffer(h->avctx, &pic->tf, pic->reference ?
>> - AV_GET_BUFFER_FLAG_REF : 0);
>> - if (ret < 0)
>> - goto fail;
>> +
>> + if (h->avctx->pix_fmt == AV_PIX_FMT_VDA) {
>> + /* the VDA framework handles its surfaces internally, so we
>> + * do not call get_buffer at all;
>> + * create a dummy buffer here until we get the real one later */
>> + pic->f.buf[0] = av_buffer_alloc(1);
>> + pic->f.width = h->avctx->width;
>> + pic->f.height = h->avctx->height;
>> + pic->f.format = h->avctx->pix_fmt;
>> + if (!pic->f.buf[0])
>> + goto fail;
>> + } else {
>> + ret = ff_thread_get_buffer(h->avctx, &pic->tf, pic->reference ?
>> + AV_GET_BUFFER_FLAG_REF : 0);
>> + if (ret < 0)
>> + goto fail;
>> + }
>
> Maybe this could be made a bit nicer by adding a new callback to
> AVHWAccel? If that callback is NULL, the code would fall back to
> ff_thread_get_buffer.
>

Surely we can and we should IMHO.

lu
Luca Barbato
2014-03-24 02:01:54 UTC
Permalink
From: Anton Khirnov <***@khirnov.net>

Signed-off-by: Luca Barbato <***@gentoo.org>
---
Makefile | 1 +
avconv.h | 2 +
avconv_opt.c | 3 ++
avconv_vda.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 140 insertions(+)
create mode 100644 avconv_vda.c

diff --git a/Makefile b/Makefile
index 2453dfd..8836d4f 100644
--- a/Makefile
+++ b/Makefile
@@ -75,6 +75,7 @@ $(foreach prog,$(AVBASENAMES),$(eval OBJS-$(prog) += cmdutils.o))

OBJS-avconv += avconv_opt.o avconv_filter.o
OBJS-avconv-$(HAVE_VDPAU_X11) += avconv_vdpau.o
+OBJS-avconv-$(CONFIG_VDA) += avconv_vda.o

TESTTOOLS = audiogen videogen rotozoom tiny_psnr base64
HOSTPROGS := $(TESTTOOLS:%=tests/%) doc/print_options
diff --git a/avconv.h b/avconv.h
index c912fae..de3fee5 100644
--- a/avconv.h
+++ b/avconv.h
@@ -52,6 +52,7 @@ enum HWAccelID {
HWACCEL_NONE = 0,
HWACCEL_AUTO,
HWACCEL_VDPAU,
+ HWACCEL_VDA,
};

typedef struct HWAccel {
@@ -403,5 +404,6 @@ FilterGraph *init_simple_filtergraph(InputStream *ist, OutputStream *ost);
int avconv_parse_options(int argc, char **argv);

int vdpau_init(AVCodecContext *s);
+int vda_init(AVCodecContext *s);

#endif /* AVCONV_H */
diff --git a/avconv_opt.c b/avconv_opt.c
index 7bc41c9..e561763 100644
--- a/avconv_opt.c
+++ b/avconv_opt.c
@@ -57,6 +57,9 @@ const HWAccel hwaccels[] = {
#if HAVE_VDPAU_X11
{ "vdpau", vdpau_init, HWACCEL_VDPAU, AV_PIX_FMT_VDPAU },
#endif
+#if CONFIG_VDA
+ { "vda", vda_init, HWACCEL_VDA, AV_PIX_FMT_VDA },
+#endif
{ 0 },
};

diff --git a/avconv_vda.c b/avconv_vda.c
new file mode 100644
index 0000000..40f87c4
--- /dev/null
+++ b/avconv_vda.c
@@ -0,0 +1,134 @@
+/*
+ * This file is part of Libav.
+ *
+ * Libav is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * Libav is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with Libav; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavcodec/avcodec.h"
+#include "libavcodec/vda.h"
+#include "libavutil/imgutils.h"
+
+#include "avconv.h"
+
+typedef struct VDAContext {
+ AVFrame *tmp_frame;
+} VDAContext;
+
+static int vda_retrieve_data(AVCodecContext *s, AVFrame *frame)
+{
+ InputStream *ist = s->opaque;
+ VDAContext *vda = ist->hwaccel_ctx;
+ CVPixelBufferRef pixbuf = (CVPixelBufferRef)frame->data[3];
+ OSType pixel_format = CVPixelBufferGetPixelFormatType(pixbuf);
+ CVReturn err;
+ uint8_t *data[4] = { 0 };
+ int linesize[4] = { 0 };
+ int planes, ret, i;
+
+ av_frame_unref(vda->tmp_frame);
+
+ switch (pixel_format) {
+ case kCVPixelFormatType_420YpCbCr8Planar: vda->tmp_frame->format = AV_PIX_FMT_YUV420P; break;
+ case kCVPixelFormatType_422YpCbCr8: vda->tmp_frame->format = AV_PIX_FMT_UYVY422; break;
+ default:
+ av_log(NULL, AV_LOG_ERROR,
+ "Unsupported pixel format: %u\n", pixel_format);
+ return AVERROR(ENOSYS);
+ }
+
+ vda->tmp_frame->width = frame->width;
+ vda->tmp_frame->height = frame->height;
+ ret = av_frame_get_buffer(vda->tmp_frame, 32);
+ if (ret < 0)
+ return ret;
+
+ err = CVPixelBufferLockBaseAddress(pixbuf, kCVPixelBufferLock_ReadOnly);
+ if (err != kCVReturnSuccess) {
+ av_log(NULL, AV_LOG_ERROR, "Error locking the pixel buffer.\n");
+ return AVERROR_UNKNOWN;
+ }
+
+ if (CVPixelBufferIsPlanar(pixbuf)) {
+
+ planes = CVPixelBufferGetPlaneCount(pixbuf);
+ for (i = 0; i < planes; i++) {
+ data[i] = CVPixelBufferGetBaseAddressOfPlane(pixbuf, i);
+ linesize[i] = CVPixelBufferGetBytesPerRowOfPlane(pixbuf, i);
+ }
+ } else {
+ data[0] = CVPixelBufferGetBaseAddress(pixbuf);
+ linesize[0] = CVPixelBufferGetBytesPerRow(pixbuf);
+ }
+
+ av_image_copy(vda->tmp_frame->data, vda->tmp_frame->linesize,
+ data, linesize, vda->tmp_frame->format,
+ frame->width, frame->height);
+
+ ret = av_frame_copy_props(vda->tmp_frame, frame);
+ if (ret < 0)
+ return ret;
+
+ av_frame_unref(frame);
+ av_frame_move_ref(frame, vda->tmp_frame);
+
+ return 0;
+}
+
+static void vda_uninit(AVCodecContext *s)
+{
+ InputStream *ist = s->opaque;
+ VDAContext *vda = ist->hwaccel_ctx;
+
+ ist->hwaccel_uninit = NULL;
+ ist->hwaccel_retrieve_data = NULL;
+
+ av_frame_free(&vda->tmp_frame);
+
+ av_vda_default_free(s);
+ av_freep(&ist->hwaccel_ctx);
+}
+
+int vda_init(AVCodecContext *s)
+{
+ InputStream *ist = s->opaque;
+ int loglevel = (ist->hwaccel_id == HWACCEL_AUTO) ? AV_LOG_VERBOSE : AV_LOG_ERROR;
+ VDAContext *vda;
+ int ret;
+
+ vda = av_mallocz(sizeof(*vda));
+ if (!vda)
+ return AVERROR(ENOMEM);
+
+ ist->hwaccel_ctx = vda;
+ ist->hwaccel_uninit = vda_uninit;
+ ist->hwaccel_retrieve_data = vda_retrieve_data;
+
+ vda->tmp_frame = av_frame_alloc();
+ if (!vda->tmp_frame) {
+ ret = AVERROR(ENOMEM);
+ goto fail;
+ }
+
+ ret = av_vda_default_init(s);
+ if (ret < 0) {
+ av_log(NULL, loglevel, "Error creating VDA decoder.\n");
+ goto fail;
+ }
+
+ return 0;
+fail:
+ vda_uninit(s);
+ return ret;
+}
--
1.8.5.2 (Apple Git-48)
Diego Biurrun
2014-03-25 14:50:03 UTC
Permalink
On Mon, Mar 24, 2014 at 03:01:54AM +0100, Luca Barbato wrote:
> --- a/Makefile
> +++ b/Makefile
> @@ -75,6 +75,7 @@ $(foreach prog,$(AVBASENAMES),$(eval OBJS-$(prog) += cmdutils.o))
>
> OBJS-avconv += avconv_opt.o avconv_filter.o
> OBJS-avconv-$(HAVE_VDPAU_X11) += avconv_vdpau.o
> +OBJS-avconv-$(CONFIG_VDA) += avconv_vda.o

order

> --- /dev/null
> +++ b/avconv_vda.c
> @@ -0,0 +1,134 @@
> +
> +#include "libavcodec/avcodec.h"
> +#include "libavcodec/vda.h"
> +#include "libavutil/imgutils.h"
> +
> +#include "avconv.h"

canonical header order

Diego
Anton Khirnov
2014-03-24 09:03:16 UTC
Permalink
On Mon, 24 Mar 2014 08:46:01 +0100, Rémi Denis-Courmont <***@remlab.net> wrote:
> On Mon, 24 Mar 2014 03:01:46 +0100, Luca Barbato <***@gentoo.org>
> wrote:
> > From: Anton Khirnov <***@khirnov.net>
> >
> > This way each decoder does not have to do the same thing manually.
> > ---
> > libavcodec/h263dec.c | 1 -
> > libavcodec/h264_slice.c | 2 --
> > libavcodec/internal.h | 9 ---------
> > libavcodec/mpeg12dec.c | 2 --
> > libavcodec/utils.c | 48
> > +++++++++++++++++++++++++++++++++---------------
> > libavcodec/vc1dec.c | 1 -
> > 6 files changed, 33 insertions(+), 30 deletions(-)
> >
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index 2b3c00a..e52d915 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -836,9 +836,41 @@ enum AVPixelFormat
> avcodec_default_get_format(struct
> > AVCodecContext *s, const en
> > return fmt[0];
> > }
> >
> > +static AVHWAccel *find_hwaccel(enum AVCodecID codec_id,
> > + enum AVPixelFormat pix_fmt)
> > +{
> > + AVHWAccel *hwaccel = NULL;
> > +
> > + while ((hwaccel = av_hwaccel_next(hwaccel)))
> > + if (hwaccel->id == codec_id
> > + && hwaccel->pix_fmt == pix_fmt)
> > + return hwaccel;
> > + return NULL;
> > +}
> > +
> > +
> > int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
> > {
> > - return avctx->get_format(avctx, fmt);
> > + const AVPixFmtDescriptor *desc;
> > + enum AVPixelFormat ret = avctx->get_format(avctx, fmt);
> > +
> > + desc = av_pix_fmt_desc_get(ret);
> > + if (!desc)
> > + return AV_PIX_FMT_NONE;
> > +
> > + if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
> > + avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
> > + if (!avctx->hwaccel) {
> > + av_log(avctx, AV_LOG_ERROR,
> > + "Could not find an AVHWAccel for the pixel format:
> %s",
> > + desc->name);
> > + return AV_PIX_FMT_NONE;
>
> Is this recoverable in any reasonably way?
>
> I can see three ways this situation happens:
>
> 1) libavcodec advertised a pxiel format in get_format() that it does not
> actually support. That would be a bug in libavcodec (this bug actually does
> exist if you disable some hwaccel manually in ./configure).
>
> 2) The application returned a pixel format that was not advertised. I
> think that is a non-recoverable application bug.
>

Yes, those two are bugs that should be fixed in their respective places, and
in the following we can assume they do not happen.

> 3) Initialization of the hwaccel back-end unexpectedly fail. I am not sure
> if this should be allowed at this step. But if it should, I think the
> correct solution is to call get_format again, *without* the failed pixel
> format in the list. That way the application has a chance to recover by
> falling back to some other format.
>

With this patchset, the only reason initialization can fail is out of memory.
And that's not something we can easily recover from.

--
Anton Khirnov
wm4
2014-03-24 19:25:09 UTC
Permalink
On Mon, 24 Mar 2014 10:03:16 +0100
Anton Khirnov <***@khirnov.net> wrote:

>
> On Mon, 24 Mar 2014 08:46:01 +0100, Rémi Denis-Courmont <***@remlab.net> wrote:
> > On Mon, 24 Mar 2014 03:01:46 +0100, Luca Barbato <***@gentoo.org>
> > wrote:
> > > From: Anton Khirnov <***@khirnov.net>
> > >
> > > This way each decoder does not have to do the same thing manually.
> > > ---
> > > libavcodec/h263dec.c | 1 -
> > > libavcodec/h264_slice.c | 2 --
> > > libavcodec/internal.h | 9 ---------
> > > libavcodec/mpeg12dec.c | 2 --
> > > libavcodec/utils.c | 48
> > > +++++++++++++++++++++++++++++++++---------------
> > > libavcodec/vc1dec.c | 1 -
> > > 6 files changed, 33 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > > index 2b3c00a..e52d915 100644
> > > --- a/libavcodec/utils.c
> > > +++ b/libavcodec/utils.c
> > > @@ -836,9 +836,41 @@ enum AVPixelFormat
> > avcodec_default_get_format(struct
> > > AVCodecContext *s, const en
> > > return fmt[0];
> > > }
> > >
> > > +static AVHWAccel *find_hwaccel(enum AVCodecID codec_id,
> > > + enum AVPixelFormat pix_fmt)
> > > +{
> > > + AVHWAccel *hwaccel = NULL;
> > > +
> > > + while ((hwaccel = av_hwaccel_next(hwaccel)))
> > > + if (hwaccel->id == codec_id
> > > + && hwaccel->pix_fmt == pix_fmt)
> > > + return hwaccel;
> > > + return NULL;
> > > +}
> > > +
> > > +
> > > int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
> > > {
> > > - return avctx->get_format(avctx, fmt);
> > > + const AVPixFmtDescriptor *desc;
> > > + enum AVPixelFormat ret = avctx->get_format(avctx, fmt);
> > > +
> > > + desc = av_pix_fmt_desc_get(ret);
> > > + if (!desc)
> > > + return AV_PIX_FMT_NONE;
> > > +
> > > + if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
> > > + avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
> > > + if (!avctx->hwaccel) {
> > > + av_log(avctx, AV_LOG_ERROR,
> > > + "Could not find an AVHWAccel for the pixel format:
> > %s",
> > > + desc->name);
> > > + return AV_PIX_FMT_NONE;
> >
> > Is this recoverable in any reasonably way?
> >
> > I can see three ways this situation happens:
> >
> > 1) libavcodec advertised a pxiel format in get_format() that it does not
> > actually support. That would be a bug in libavcodec (this bug actually does
> > exist if you disable some hwaccel manually in ./configure).
> >
> > 2) The application returned a pixel format that was not advertised. I
> > think that is a non-recoverable application bug.
> >
>
> Yes, those two are bugs that should be fixed in their respective places, and
> in the following we can assume they do not happen.

I use separate AVCodecContexts for software and hardware decoding. If
initializing hardware decoding somehow fails, I return AV_PIX_FMT_NONE
from get_format to make the decoder return as soon as possible. Pretty
violent, but works well.

> > 3) Initialization of the hwaccel back-end unexpectedly fail. I am not sure
> > if this should be allowed at this step. But if it should, I think the
> > correct solution is to call get_format again, *without* the failed pixel
> > format in the list. That way the application has a chance to recover by
> > falling back to some other format.
> >
>
> With this patchset, the only reason initialization can fail is out of memory.
> And that's not something we can easily recover from.
>
Anton Khirnov
2014-03-24 19:34:53 UTC
Permalink
On Mon, 24 Mar 2014 20:25:09 +0100, wm4 <***@googlemail.com> wrote:
> On Mon, 24 Mar 2014 10:03:16 +0100
> Anton Khirnov <***@khirnov.net> wrote:
>
> >
> > On Mon, 24 Mar 2014 08:46:01 +0100, Rémi Denis-Courmont <***@remlab.net> wrote:
> > > On Mon, 24 Mar 2014 03:01:46 +0100, Luca Barbato <***@gentoo.org>
> > > wrote:
> > > > From: Anton Khirnov <***@khirnov.net>
> > > >
> > > > This way each decoder does not have to do the same thing manually.
> > > > ---
> > > > libavcodec/h263dec.c | 1 -
> > > > libavcodec/h264_slice.c | 2 --
> > > > libavcodec/internal.h | 9 ---------
> > > > libavcodec/mpeg12dec.c | 2 --
> > > > libavcodec/utils.c | 48
> > > > +++++++++++++++++++++++++++++++++---------------
> > > > libavcodec/vc1dec.c | 1 -
> > > > 6 files changed, 33 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > > > index 2b3c00a..e52d915 100644
> > > > --- a/libavcodec/utils.c
> > > > +++ b/libavcodec/utils.c
> > > > @@ -836,9 +836,41 @@ enum AVPixelFormat
> > > avcodec_default_get_format(struct
> > > > AVCodecContext *s, const en
> > > > return fmt[0];
> > > > }
> > > >
> > > > +static AVHWAccel *find_hwaccel(enum AVCodecID codec_id,
> > > > + enum AVPixelFormat pix_fmt)
> > > > +{
> > > > + AVHWAccel *hwaccel = NULL;
> > > > +
> > > > + while ((hwaccel = av_hwaccel_next(hwaccel)))
> > > > + if (hwaccel->id == codec_id
> > > > + && hwaccel->pix_fmt == pix_fmt)
> > > > + return hwaccel;
> > > > + return NULL;
> > > > +}
> > > > +
> > > > +
> > > > int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
> > > > {
> > > > - return avctx->get_format(avctx, fmt);
> > > > + const AVPixFmtDescriptor *desc;
> > > > + enum AVPixelFormat ret = avctx->get_format(avctx, fmt);
> > > > +
> > > > + desc = av_pix_fmt_desc_get(ret);
> > > > + if (!desc)
> > > > + return AV_PIX_FMT_NONE;
> > > > +
> > > > + if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
> > > > + avctx->hwaccel = find_hwaccel(avctx->codec_id, ret);
> > > > + if (!avctx->hwaccel) {
> > > > + av_log(avctx, AV_LOG_ERROR,
> > > > + "Could not find an AVHWAccel for the pixel format:
> > > %s",
> > > > + desc->name);
> > > > + return AV_PIX_FMT_NONE;
> > >
> > > Is this recoverable in any reasonably way?
> > >
> > > I can see three ways this situation happens:
> > >
> > > 1) libavcodec advertised a pxiel format in get_format() that it does not
> > > actually support. That would be a bug in libavcodec (this bug actually does
> > > exist if you disable some hwaccel manually in ./configure).
> > >
> > > 2) The application returned a pixel format that was not advertised. I
> > > think that is a non-recoverable application bug.
> > >
> >
> > Yes, those two are bugs that should be fixed in their respective places, and
> > in the following we can assume they do not happen.
>
> I use separate AVCodecContexts for software and hardware decoding. If
> initializing hardware decoding somehow fails, I return AV_PIX_FMT_NONE
> from get_format to make the decoder return as soon as possible. Pretty
> violent, but works well.

I think the question was whether we can recover from failures in this function
itself.

When the caller get_format() fails, there is nothing libavcodec can reasonably
do.

--
Anton Khirnov
Anton Khirnov
2014-03-24 20:28:32 UTC
Permalink
On Mon, 24 Mar 2014 21:04:09 +0100, wm4 <***@googlemail.com> wrote:
> On Mon, 24 Mar 2014 03:01:51 +0100
> Luca Barbato <***@gentoo.org> wrote:
>
>
> > + AV_PIX_FMT_VDA, ///< HW acceleration through VDA, data[3] contains a CVPixelBufferRef
>
> I keep wondering about this: why data[3]? data[0] would be a little bit
> less strange. Is this due to historical reasons (hw surfaces can be 0,
> and data[0] being NULL did not work due to now removed checks), or
> consistency with older hwaccels, or something else?

I do not know what the original reason for it was, now i'm just doing it for
consistency.

--
Anton Khirnov
Anton Khirnov
2014-04-17 18:26:39 UTC
Permalink
On Mon, 24 Mar 2014 03:21:43 +0100, Vittorio Giovara <***@gmail.com> wrote:
> On Mon, Mar 24, 2014 at 3:01 AM, Luca Barbato <***@gentoo.org> wrote:
> > From: Anton Khirnov <***@khirnov.net>
> >
> > This describes more accurately what this field is for.
> > ---
> > libavcodec/avcodec.h | 4 ++--
> > libavcodec/dxva2_h264.c | 2 +-
> > libavcodec/dxva2_mpeg2.c | 2 +-
> > libavcodec/dxva2_vc1.c | 4 ++--
> > libavcodec/h264_slice.c | 4 ++--
> > libavcodec/mpegvideo.c | 4 ++--
> > libavcodec/vdpau_h264.c | 2 +-
> > libavcodec/vdpau_mpeg12.c | 4 ++--
> > libavcodec/vdpau_mpeg4.c | 4 ++--
> > libavcodec/vdpau_vc1.c | 4 ++--
> > 10 files changed, 17 insertions(+), 17 deletions(-)
> >
>
> Probably OK but I think you should bump avcodec's minor.

Why?

Either we consider this field public (can't imagine a legitimate use case for it
though), then it's a major bump.

Or we don't, then there's no bump.

--
Anton Khirnov
Anton Khirnov
2014-04-17 18:28:19 UTC
Permalink
On Mon, 24 Mar 2014 03:01:49 +0100, Luca Barbato <***@gentoo.org> wrote:
> ---
> libavcodec/pthread_frame.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 1af8ff5..558a518 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -206,6 +206,7 @@ static int update_context_from_thread(AVCodecContext *dst, AVCodecContext *src,
>
> dst->hwaccel = src->hwaccel;
> dst->hwaccel_context = src->hwaccel_context;
> + dst->internal->hwaccel_priv_data = src->internal->hwaccel_priv_data;

The code looks ok, but the patch is out of order and should be squashed with the
following commit.

--
Anton Khirnov
Anton Khirnov
2014-04-17 18:41:50 UTC
Permalink
On Mon, 24 Mar 2014 03:01:52 +0100, Luca Barbato <***@gentoo.org> wrote:
> From: Anton Khirnov <***@khirnov.net>
>
> ---
> libavcodec/vda.h | 6 +++---
> libavcodec/vda_h264.c | 50 +++++++++++++++++++++++++++++++++++---------------
> 2 files changed, 38 insertions(+), 18 deletions(-)
>

I can hardly review my own code, so someone else will have to do it.

--
Anton Khirnov
Luca Barbato
2014-04-17 18:43:46 UTC
Permalink
On 17/04/14 20:41, Anton Khirnov wrote:
>
> On Mon, 24 Mar 2014 03:01:52 +0100, Luca Barbato <***@gentoo.org> wrote:
>> From: Anton Khirnov <***@khirnov.net>
>>
>> ---
>> libavcodec/vda.h | 6 +++---
>> libavcodec/vda_h264.c | 50 +++++++++++++++++++++++++++++++++++---------------
>> 2 files changed, 38 insertions(+), 18 deletions(-)
>>
>
> I can hardly review my own code, so someone else will have to do it.
>
It is ok for me with the fixes I wrote and I tested on mpv and vlc already.

Felix and Stefano had been polled about it already.

lu
Continue reading on narkive:
Loading...