diff mbox series

[FFmpeg-devel] lavu/hwcontext_qsv: add 10-bit RGB support for QSV

Message ID 20200809051107.86008-1-linjie.justin.fu@gmail.com
State New
Headers show
Series [FFmpeg-devel] lavu/hwcontext_qsv: add 10-bit RGB support for QSV
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Linjie Fu Aug. 9, 2020, 5:11 a.m. UTC
After adding a copy pass through path inside MSDK, x2rgb10 is now
available as an output of VPP.

Command line for CSC:
ffmpeg -hwaccel qsv -v verbose -c:v hevc_qsv -i
    p010.h265 -vf scale_qsv=format=x2rgb10,hwdownload,format=x2rgb10
    -vframes 1 out.yuv

Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
---
[1]Issue:
https://github.com/Intel-Media-SDK/MediaSDK/issues/1654
[2]Adding copy passthrou:
https://github.com/AntonGrishin/MediaSDK/commit/20058c0e1dbd521e2571e14f6246c48fea996094
[3]Still in need to be fixed:
https://github.com/Intel-Media-SDK/MediaSDK/pull/2268

This patch would be ready to merge after [3] is handled decently.

 libavutil/hwcontext_qsv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Mark Thompson Aug. 9, 2020, 8:31 p.m. UTC | #1
On 09/08/2020 06:11, Linjie Fu wrote:
> After adding a copy pass through path inside MSDK, x2rgb10 is now
> available as an output of VPP.
> 
> Command line for CSC:
> ffmpeg -hwaccel qsv -v verbose -c:v hevc_qsv -i
>      p010.h265 -vf scale_qsv=format=x2rgb10,hwdownload,format=x2rgb10
>      -vframes 1 out.yuv
> 
> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> ---
> [1]Issue:
> https://github.com/Intel-Media-SDK/MediaSDK/issues/1654
> [2]Adding copy passthrou:
> https://github.com/AntonGrishin/MediaSDK/commit/20058c0e1dbd521e2571e14f6246c48fea996094
> [3]Still in need to be fixed:
> https://github.com/Intel-Media-SDK/MediaSDK/pull/2268
> 
> This patch would be ready to merge after [3] is handled decently.
> 
>   libavutil/hwcontext_qsv.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index 35a944f8f8..64cb1663d8 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -110,6 +110,10 @@ static const struct {
>   #if CONFIG_VAAPI
>       { AV_PIX_FMT_YUYV422,
>                          MFX_FOURCC_YUY2 },
> +#if QSV_VERSION_ATLEAST(1, 9)
> +    { AV_PIX_FMT_X2RGB10,
> +                    MFX_FOURCC_A2RGB10 },
> +#endif
>   #if QSV_VERSION_ATLEAST(1, 27)
>       { AV_PIX_FMT_Y210,
>                          MFX_FOURCC_Y210 },
> @@ -798,6 +802,10 @@ static int map_frame_to_surface(const AVFrame *frame, mfxFrameSurface1 *surface)
>           surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
>           break;
>   #endif
> +    case AV_PIX_FMT_X2RGB10:
> +        surface->Data.A2RGB10 = (mfxA2RGB10 *)frame->data[0];
> +        break;
> +
>       default:
>           return MFX_ERR_UNSUPPORTED;
>       }
> 

What happens to the alpha information here?  We don't specify what is in the unused bits of X2RGB10, so saying that it has an alpha component seems likely to do something ugly in cases where that is used (e.g. overlay).

- Mark
Linjie Fu Aug. 10, 2020, 2:58 a.m. UTC | #2
On Mon, Aug 10, 2020 at 4:31 AM Mark Thompson <sw@jkqxz.net> wrote:
>
> On 09/08/2020 06:11, Linjie Fu wrote:
> > After adding a copy pass through path inside MSDK, x2rgb10 is now
> > available as an output of VPP.
> >
> > Command line for CSC:
> > ffmpeg -hwaccel qsv -v verbose -c:v hevc_qsv -i
> >      p010.h265 -vf scale_qsv=format=x2rgb10,hwdownload,format=x2rgb10
> >      -vframes 1 out.yuv
> >
> > Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> > ---
> > [1]Issue:
> > https://github.com/Intel-Media-SDK/MediaSDK/issues/1654
> > [2]Adding copy passthrou:
> > https://github.com/AntonGrishin/MediaSDK/commit/20058c0e1dbd521e2571e14f6246c48fea996094
> > [3]Still in need to be fixed:
> > https://github.com/Intel-Media-SDK/MediaSDK/pull/2268
> >
> > This patch would be ready to merge after [3] is handled decently.
> >
> >   libavutil/hwcontext_qsv.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> > index 35a944f8f8..64cb1663d8 100644
> > --- a/libavutil/hwcontext_qsv.c
> > +++ b/libavutil/hwcontext_qsv.c
> > @@ -110,6 +110,10 @@ static const struct {
> >   #if CONFIG_VAAPI
> >       { AV_PIX_FMT_YUYV422,
> >                          MFX_FOURCC_YUY2 },
> > +#if QSV_VERSION_ATLEAST(1, 9)
> > +    { AV_PIX_FMT_X2RGB10,
> > +                    MFX_FOURCC_A2RGB10 },
> > +#endif
> >   #if QSV_VERSION_ATLEAST(1, 27)
> >       { AV_PIX_FMT_Y210,
> >                          MFX_FOURCC_Y210 },
> > @@ -798,6 +802,10 @@ static int map_frame_to_surface(const AVFrame *frame, mfxFrameSurface1 *surface)
> >           surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
> >           break;
> >   #endif
> > +    case AV_PIX_FMT_X2RGB10:
> > +        surface->Data.A2RGB10 = (mfxA2RGB10 *)frame->data[0];
> > +        break;
> > +
> >       default:
> >           return MFX_ERR_UNSUPPORTED;
> >       }
> >
>
> What happens to the alpha information here?  We don't specify what is in the unused bits of X2RGB10, so saying that it has an alpha component seems likely to do something ugly in cases where that is used (e.g. overlay).

Yep, and as discussed previously, this patch is to add support for
x2rgb10(instead of pixel format a2rgb10) in ffmpeg level, which didn't
say it has the alpha component.
So I think this would be fine, since we don't care about the unused 2
bits for x2rgb10 (no matter whether it contains alpha information or
not).

- Linjie
Hendrik Leppkes Aug. 10, 2020, 6:17 a.m. UTC | #3
On Mon, Aug 10, 2020 at 6:07 AM Linjie Fu <linjie.justin.fu@gmail.com> wrote:
>
> On Mon, Aug 10, 2020 at 4:31 AM Mark Thompson <sw@jkqxz.net> wrote:
> >
> > On 09/08/2020 06:11, Linjie Fu wrote:
> > > After adding a copy pass through path inside MSDK, x2rgb10 is now
> > > available as an output of VPP.
> > >
> > > Command line for CSC:
> > > ffmpeg -hwaccel qsv -v verbose -c:v hevc_qsv -i
> > >      p010.h265 -vf scale_qsv=format=x2rgb10,hwdownload,format=x2rgb10
> > >      -vframes 1 out.yuv
> > >
> > > Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> > > ---
> > > [1]Issue:
> > > https://github.com/Intel-Media-SDK/MediaSDK/issues/1654
> > > [2]Adding copy passthrou:
> > > https://github.com/AntonGrishin/MediaSDK/commit/20058c0e1dbd521e2571e14f6246c48fea996094
> > > [3]Still in need to be fixed:
> > > https://github.com/Intel-Media-SDK/MediaSDK/pull/2268
> > >
> > > This patch would be ready to merge after [3] is handled decently.
> > >
> > >   libavutil/hwcontext_qsv.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > >
> > > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> > > index 35a944f8f8..64cb1663d8 100644
> > > --- a/libavutil/hwcontext_qsv.c
> > > +++ b/libavutil/hwcontext_qsv.c
> > > @@ -110,6 +110,10 @@ static const struct {
> > >   #if CONFIG_VAAPI
> > >       { AV_PIX_FMT_YUYV422,
> > >                          MFX_FOURCC_YUY2 },
> > > +#if QSV_VERSION_ATLEAST(1, 9)
> > > +    { AV_PIX_FMT_X2RGB10,
> > > +                    MFX_FOURCC_A2RGB10 },
> > > +#endif
> > >   #if QSV_VERSION_ATLEAST(1, 27)
> > >       { AV_PIX_FMT_Y210,
> > >                          MFX_FOURCC_Y210 },
> > > @@ -798,6 +802,10 @@ static int map_frame_to_surface(const AVFrame *frame, mfxFrameSurface1 *surface)
> > >           surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
> > >           break;
> > >   #endif
> > > +    case AV_PIX_FMT_X2RGB10:
> > > +        surface->Data.A2RGB10 = (mfxA2RGB10 *)frame->data[0];
> > > +        break;
> > > +
> > >       default:
> > >           return MFX_ERR_UNSUPPORTED;
> > >       }
> > >
> >
> > What happens to the alpha information here?  We don't specify what is in the unused bits of X2RGB10, so saying that it has an alpha component seems likely to do something ugly in cases where that is used (e.g. overlay).
>
> Yep, and as discussed previously, this patch is to add support for
> x2rgb10(instead of pixel format a2rgb10) in ffmpeg level, which didn't
> say it has the alpha component.
> So I think this would be fine, since we don't care about the unused 2
> bits for x2rgb10 (no matter whether it contains alpha information or
> not).

I think Marks point is that the QSV format is called
MFX_FOURCC_A2RGB10, eg. A2RGB10, not X2RGB10. It sounds like the QSV
format expects alpha to be valid.

It should be clarified what happens with the alpha, and made sure that
it doesnt produce files with an alpha channel full of nonsense.

- Hendrik
diff mbox series

Patch

diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index 35a944f8f8..64cb1663d8 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -110,6 +110,10 @@  static const struct {
 #if CONFIG_VAAPI
     { AV_PIX_FMT_YUYV422,
                        MFX_FOURCC_YUY2 },
+#if QSV_VERSION_ATLEAST(1, 9)
+    { AV_PIX_FMT_X2RGB10,
+                    MFX_FOURCC_A2RGB10 },
+#endif
 #if QSV_VERSION_ATLEAST(1, 27)
     { AV_PIX_FMT_Y210,
                        MFX_FOURCC_Y210 },
@@ -798,6 +802,10 @@  static int map_frame_to_surface(const AVFrame *frame, mfxFrameSurface1 *surface)
         surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
         break;
 #endif
+    case AV_PIX_FMT_X2RGB10:
+        surface->Data.A2RGB10 = (mfxA2RGB10 *)frame->data[0];
+        break;
+
     default:
         return MFX_ERR_UNSUPPORTED;
     }