diff mbox series

[FFmpeg-devel,1/2] lavu/hwcontext_qsv: add support for AV_PIX_FMT_VUYX

Message ID 20220829072751.22395-1-haihao.xiang@intel.com
State New
Headers show
Series [FFmpeg-devel,1/2] lavu/hwcontext_qsv: add support for AV_PIX_FMT_VUYX | expand

Checks

Context Check Description
andriy/configure_x86 warning Failed to run configure

Commit Message

Xiang, Haihao Aug. 29, 2022, 7:27 a.m. UTC
From: Haihao Xiang <haihao.xiang@intel.com>

AV_PIX_FMT_VUYX is used in FFmpeg for 8bit 4:4:4 content on Intel HW,
and MFX_FOURCC_AYUV is used in the SDK
---
 libavutil/hwcontext_qsv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

James Almer Aug. 29, 2022, 11:17 a.m. UTC | #1
On 8/29/2022 4:27 AM, Xiang, Haihao wrote:
> From: Haihao Xiang <haihao.xiang@intel.com>
> 
> AV_PIX_FMT_VUYX is used in FFmpeg for 8bit 4:4:4 content on Intel HW,
> and MFX_FOURCC_AYUV is used in the SDK

Sounds like you want the VUYA pixfmt instead.

> ---
>   libavutil/hwcontext_qsv.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index 510f422562..a3350eae0f 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -119,6 +119,8 @@ static const struct {
>                          MFX_FOURCC_YUY2 },
>       { AV_PIX_FMT_Y210,
>                          MFX_FOURCC_Y210 },
> +    { AV_PIX_FMT_VUYX,
> +                       MFX_FOURCC_AYUV },
>   #endif
>   };
>   
> @@ -1502,6 +1504,12 @@ static int map_frame_to_surface(const AVFrame *frame, mfxFrameSurface1 *surface)
>           surface->Data.U16 = (mfxU16 *)frame->data[0] + 1;
>           surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
>           break;
> +    case AV_PIX_FMT_VUYX:
> +        surface->Data.V = frame->data[0];
> +        surface->Data.U = frame->data[0] + 1;
> +        surface->Data.Y = frame->data[0] + 2;
> +        surface->Data.A = frame->data[0] + 3;

This will go wrong with VUYX. You need to use AV_PIX_FMT_VUYA.

> +        break;
>   #endif
>       default:
>           return MFX_ERR_UNSUPPORTED;
Xiang, Haihao Aug. 29, 2022, 2:01 p.m. UTC | #2
On Mon, 2022-08-29 at 08:17 -0300, James Almer wrote:
> On 8/29/2022 4:27 AM, Xiang, Haihao wrote:
> > From: Haihao Xiang <haihao.xiang@intel.com>
> > 
> > AV_PIX_FMT_VUYX is used in FFmpeg for 8bit 4:4:4 content on Intel HW,
> > and MFX_FOURCC_AYUV is used in the SDK
> 
> Sounds like you want the VUYA pixfmt instead.

AV_PIX_FMT_VUYX is used for 4:4:4 content in the VAAPI path, AV_PIX_FMT_VUYX is
expected when creating vaapi urface. QSV is based on 
VAAPI under Linux, so AV_PIX_FMT_VUYX is expected too in the QSV path when
creating vaapi surface.

> 
> > ---
> >   libavutil/hwcontext_qsv.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> > index 510f422562..a3350eae0f 100644
> > --- a/libavutil/hwcontext_qsv.c
> > +++ b/libavutil/hwcontext_qsv.c
> > @@ -119,6 +119,8 @@ static const struct {
> >                          MFX_FOURCC_YUY2 },
> >       { AV_PIX_FMT_Y210,
> >                          MFX_FOURCC_Y210 },
> > +    { AV_PIX_FMT_VUYX,
> > +                       MFX_FOURCC_AYUV },
> >   #endif
> >   };
> >   
> > @@ -1502,6 +1504,12 @@ static int map_frame_to_surface(const AVFrame *frame,
> > mfxFrameSurface1 *surface)
> >           surface->Data.U16 = (mfxU16 *)frame->data[0] + 1;
> >           surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
> >           break;
> > +    case AV_PIX_FMT_VUYX:
> > +        surface->Data.V = frame->data[0];
> > +        surface->Data.U = frame->data[0] + 1;
> > +        surface->Data.Y = frame->data[0] + 2;
> > +        surface->Data.A = frame->data[0] + 3;
> 
> This will go wrong with VUYX. You need to use AV_PIX_FMT_VUYA.

frame->data[0] + 3 is valid even if alpha channel is ignored in VUYX. Intel HW
doesn't use the data in alpha channel actually, but the SDK uses Microsoft pixel
format AYUV which is the alpha version, here set a valid address to alpha
channel when mapping a VUYX AVFrame to a AYUV mfx surface. 

Thanks
Haihao

> 
> > +        break;
> >   #endif
> >       default:
> >           return MFX_ERR_UNSUPPORTED;
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Xiang, Haihao Sept. 5, 2022, 2:47 a.m. UTC | #3
On Mon, 2022-08-29 at 14:01 +0000, Xiang, Haihao wrote:
> On Mon, 2022-08-29 at 08:17 -0300, James Almer wrote:
> > On 8/29/2022 4:27 AM, Xiang, Haihao wrote:
> > > From: Haihao Xiang <haihao.xiang@intel.com>
> > > 
> > > AV_PIX_FMT_VUYX is used in FFmpeg for 8bit 4:4:4 content on Intel HW,
> > > and MFX_FOURCC_AYUV is used in the SDK
> > 
> > Sounds like you want the VUYA pixfmt instead.
> 
> AV_PIX_FMT_VUYX is used for 4:4:4 content in the VAAPI path, AV_PIX_FMT_VUYX
> is
> expected when creating vaapi urface. QSV is based on 
> VAAPI under Linux, so AV_PIX_FMT_VUYX is expected too in the QSV path when
> creating vaapi surface.
> 
> > 
> > > ---
> > >   libavutil/hwcontext_qsv.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> > > index 510f422562..a3350eae0f 100644
> > > --- a/libavutil/hwcontext_qsv.c
> > > +++ b/libavutil/hwcontext_qsv.c
> > > @@ -119,6 +119,8 @@ static const struct {
> > >                          MFX_FOURCC_YUY2 },
> > >       { AV_PIX_FMT_Y210,
> > >                          MFX_FOURCC_Y210 },
> > > +    { AV_PIX_FMT_VUYX,
> > > +                       MFX_FOURCC_AYUV },
> > >   #endif
> > >   };
> > >   
> > > @@ -1502,6 +1504,12 @@ static int map_frame_to_surface(const AVFrame
> > > *frame,
> > > mfxFrameSurface1 *surface)
> > >           surface->Data.U16 = (mfxU16 *)frame->data[0] + 1;
> > >           surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
> > >           break;
> > > +    case AV_PIX_FMT_VUYX:
> > > +        surface->Data.V = frame->data[0];
> > > +        surface->Data.U = frame->data[0] + 1;
> > > +        surface->Data.Y = frame->data[0] + 2;
> > > +        surface->Data.A = frame->data[0] + 3;
> > 
> > This will go wrong with VUYX. You need to use AV_PIX_FMT_VUYA.
> 
> frame->data[0] + 3 is valid even if alpha channel is ignored in VUYX. Intel HW
> doesn't use the data in alpha channel actually, but the SDK uses Microsoft
> pixel
> format AYUV which is the alpha version, here set a valid address to alpha
> channel when mapping a VUYX AVFrame to a AYUV mfx surface. 
> 

Is there more comment about this patch ? 

Thanks
Haihao
Philip Langdale Sept. 5, 2022, 5:04 a.m. UTC | #4
On Mon, 5 Sep 2022 02:47:44 +0000
"Xiang, Haihao" <haihao.xiang-at-intel.com@ffmpeg.org> wrote:

> On Mon, 2022-08-29 at 14:01 +0000, Xiang, Haihao wrote:
> > On Mon, 2022-08-29 at 08:17 -0300, James Almer wrote:  
> > > On 8/29/2022 4:27 AM, Xiang, Haihao wrote:  
> > > > From: Haihao Xiang <haihao.xiang@intel.com>
> > > > 
> > > > AV_PIX_FMT_VUYX is used in FFmpeg for 8bit 4:4:4 content on
> > > > Intel HW, and MFX_FOURCC_AYUV is used in the SDK  
> > > 
> > > Sounds like you want the VUYA pixfmt instead.  
> > 
> > AV_PIX_FMT_VUYX is used for 4:4:4 content in the VAAPI path,
> > AV_PIX_FMT_VUYX is
> > expected when creating vaapi urface. QSV is based on 
> > VAAPI under Linux, so AV_PIX_FMT_VUYX is expected too in the QSV
> > path when creating vaapi surface.
> >   
> > >   
> > > > ---
> > > >   libavutil/hwcontext_qsv.c | 8 ++++++++
> > > >   1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/libavutil/hwcontext_qsv.c
> > > > b/libavutil/hwcontext_qsv.c index 510f422562..a3350eae0f 100644
> > > > --- a/libavutil/hwcontext_qsv.c
> > > > +++ b/libavutil/hwcontext_qsv.c
> > > > @@ -119,6 +119,8 @@ static const struct {
> > > >                          MFX_FOURCC_YUY2 },
> > > >       { AV_PIX_FMT_Y210,
> > > >                          MFX_FOURCC_Y210 },
> > > > +    { AV_PIX_FMT_VUYX,
> > > > +                       MFX_FOURCC_AYUV },
> > > >   #endif
> > > >   };
> > > >   
> > > > @@ -1502,6 +1504,12 @@ static int map_frame_to_surface(const
> > > > AVFrame *frame,
> > > > mfxFrameSurface1 *surface)
> > > >           surface->Data.U16 = (mfxU16 *)frame->data[0] + 1;
> > > >           surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
> > > >           break;
> > > > +    case AV_PIX_FMT_VUYX:
> > > > +        surface->Data.V = frame->data[0];
> > > > +        surface->Data.U = frame->data[0] + 1;
> > > > +        surface->Data.Y = frame->data[0] + 2;
> > > > +        surface->Data.A = frame->data[0] + 3;  
> > > 
> > > This will go wrong with VUYX. You need to use AV_PIX_FMT_VUYA.  
> > 
> > frame->data[0] + 3 is valid even if alpha channel is ignored in
> > VUYX. Intel HW doesn't use the data in alpha channel actually, but
> > the SDK uses Microsoft pixel
> > format AYUV which is the alpha version, here set a valid address to
> > alpha channel when mapping a VUYX AVFrame to a AYUV mfx surface. 
> >   
> 
> Is there more comment about this patch ? 

Wouldn't it be more correct to fill the Data.A with 0xFF rather than
using the undefined value from the frame? I assume that the encoder
actually drops the value completely so it ultimately doesn't matter,
but it makes it clear that there is no alpha and if the encoder was to
start preserving it, it would preserve the desired value.

--phil
Xiang, Haihao Sept. 5, 2022, 6:05 a.m. UTC | #5
On Sun, 2022-09-04 at 22:04 -0700, Philip Langdale wrote:
> On Mon, 5 Sep 2022 02:47:44 +0000
> "Xiang, Haihao" <haihao.xiang-at-intel.com@ffmpeg.org> wrote:
> 
> > On Mon, 2022-08-29 at 14:01 +0000, Xiang, Haihao wrote:
> > > On Mon, 2022-08-29 at 08:17 -0300, James Almer wrote:  
> > > > On 8/29/2022 4:27 AM, Xiang, Haihao wrote:  
> > > > > From: Haihao Xiang <haihao.xiang@intel.com>
> > > > > 
> > > > > AV_PIX_FMT_VUYX is used in FFmpeg for 8bit 4:4:4 content on
> > > > > Intel HW, and MFX_FOURCC_AYUV is used in the SDK  
> > > > 
> > > > Sounds like you want the VUYA pixfmt instead.  
> > > 
> > > AV_PIX_FMT_VUYX is used for 4:4:4 content in the VAAPI path,
> > > AV_PIX_FMT_VUYX is
> > > expected when creating vaapi urface. QSV is based on 
> > > VAAPI under Linux, so AV_PIX_FMT_VUYX is expected too in the QSV
> > > path when creating vaapi surface.
> > >   
> > > >   
> > > > > ---
> > > > >   libavutil/hwcontext_qsv.c | 8 ++++++++
> > > > >   1 file changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/libavutil/hwcontext_qsv.c
> > > > > b/libavutil/hwcontext_qsv.c index 510f422562..a3350eae0f 100644
> > > > > --- a/libavutil/hwcontext_qsv.c
> > > > > +++ b/libavutil/hwcontext_qsv.c
> > > > > @@ -119,6 +119,8 @@ static const struct {
> > > > >                          MFX_FOURCC_YUY2 },
> > > > >       { AV_PIX_FMT_Y210,
> > > > >                          MFX_FOURCC_Y210 },
> > > > > +    { AV_PIX_FMT_VUYX,
> > > > > +                       MFX_FOURCC_AYUV },
> > > > >   #endif
> > > > >   };
> > > > >   
> > > > > @@ -1502,6 +1504,12 @@ static int map_frame_to_surface(const
> > > > > AVFrame *frame,
> > > > > mfxFrameSurface1 *surface)
> > > > >           surface->Data.U16 = (mfxU16 *)frame->data[0] + 1;
> > > > >           surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
> > > > >           break;
> > > > > +    case AV_PIX_FMT_VUYX:
> > > > > +        surface->Data.V = frame->data[0];
> > > > > +        surface->Data.U = frame->data[0] + 1;
> > > > > +        surface->Data.Y = frame->data[0] + 2;
> > > > > +        surface->Data.A = frame->data[0] + 3;  
> > > > 
> > > > This will go wrong with VUYX. You need to use AV_PIX_FMT_VUYA.  
> > > 
> > > frame->data[0] + 3 is valid even if alpha channel is ignored in
> > > VUYX. Intel HW doesn't use the data in alpha channel actually, but
> > > the SDK uses Microsoft pixel
> > > format AYUV which is the alpha version, here set a valid address to
> > > alpha channel when mapping a VUYX AVFrame to a AYUV mfx surface. 
> > >   
> > 
> > Is there more comment about this patch ? 
> 
> Wouldn't it be more correct to fill the Data.A with 0xFF rather than
> using the undefined value from the frame? I assume that the encoder
> actually drops the value completely so it ultimately doesn't matter,
> but it makes it clear that there is no alpha and if the encoder was to
> start preserving it, it would preserve the desired value.

Yes, you're right that the encoder drops the value. I'll add some comment in the
patch. 

Thanks
Haihao
diff mbox series

Patch

diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index 510f422562..a3350eae0f 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -119,6 +119,8 @@  static const struct {
                        MFX_FOURCC_YUY2 },
     { AV_PIX_FMT_Y210,
                        MFX_FOURCC_Y210 },
+    { AV_PIX_FMT_VUYX,
+                       MFX_FOURCC_AYUV },
 #endif
 };
 
@@ -1502,6 +1504,12 @@  static int map_frame_to_surface(const AVFrame *frame, mfxFrameSurface1 *surface)
         surface->Data.U16 = (mfxU16 *)frame->data[0] + 1;
         surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
         break;
+    case AV_PIX_FMT_VUYX:
+        surface->Data.V = frame->data[0];
+        surface->Data.U = frame->data[0] + 1;
+        surface->Data.Y = frame->data[0] + 2;
+        surface->Data.A = frame->data[0] + 3;
+        break;
 #endif
     default:
         return MFX_ERR_UNSUPPORTED;