[FFmpeg-devel,v2] libavutil/hwcontext_opencl.c: fix bug in `opencl_get_plane_format`

Submitted by Jarek Samic on April 8, 2019, 2:01 a.m.

Details

Message ID 20190408020123.6439-1-cldfire3@gmail.com
State New
Headers show

Commit Message

Jarek Samic April 8, 2019, 2:01 a.m.
The `opencl_get_plane_format` function was incorrectly determining the
value used to set the image channel order. This resulted in all RGB
pixel formats being set to the `CL_RGBA` pixel format, regardless of
whether or not they actually *were* RGBA.

This patch fixes the issue by using the `offset` and depth of components
rather than the loop index to determine the value of `order`.

Signed-off-by: Jarek Samic <cldfire3@gmail.com>
---
I have updated this patch in response to the comments on the first version.
RGB is no longer special-cased, the 2, 3, and 4 mappings to `CL_R` have been
removed, and the mapping for `CL_ARGB` has been changed to the correct value.

 libavutil/hwcontext_opencl.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Mark Thompson April 8, 2019, 7:48 p.m.
On 08/04/2019 03:01, Jarek Samic wrote:
> The `opencl_get_plane_format` function was incorrectly determining the
> value used to set the image channel order. This resulted in all RGB
> pixel formats being set to the `CL_RGBA` pixel format, regardless of
> whether or not they actually *were* RGBA.
> 
> This patch fixes the issue by using the `offset` and depth of components
> rather than the loop index to determine the value of `order`.
> 
> Signed-off-by: Jarek Samic <cldfire3@gmail.com>
> ---
> I have updated this patch in response to the comments on the first version.
> RGB is no longer special-cased, the 2, 3, and 4 mappings to `CL_R` have been
> removed, and the mapping for `CL_ARGB` has been changed to the correct value.
> 
>  libavutil/hwcontext_opencl.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c
> index b116c5b708..593de1ca41 100644
> --- a/libavutil/hwcontext_opencl.c
> +++ b/libavutil/hwcontext_opencl.c
> @@ -1419,8 +1419,9 @@ static int opencl_get_plane_format(enum AVPixelFormat pixfmt,
>          // from the same component.
>          if (step && comp->step != step)
>              return AVERROR(EINVAL);
> -        order = order * 10 + c + 1;
> +
>          depth = comp->depth;
> +        order = order * 10 + comp->offset / ((depth + 7) / 8) + 1;
>          step  = comp->step;
>          alpha = (desc->flags & AV_PIX_FMT_FLAG_ALPHA &&
>                   c == desc->nb_components - 1);

This part LGTM, I can split it off and apply it on its own if you like.

> @@ -1456,14 +1457,11 @@ static int opencl_get_plane_format(enum AVPixelFormat pixfmt,
>      case order: image_format->image_channel_order = type; break;
>      switch (order) {
>          CHANNEL_ORDER(1,    CL_R);
> -        CHANNEL_ORDER(2,    CL_R);
> -        CHANNEL_ORDER(3,    CL_R);
> -        CHANNEL_ORDER(4,    CL_R);
>          CHANNEL_ORDER(12,   CL_RG);
>          CHANNEL_ORDER(23,   CL_RG);

23 should be gone too, I think?

>          CHANNEL_ORDER(1234, CL_RGBA);
> +        CHANNEL_ORDER(2341, CL_ARGB);
>          CHANNEL_ORDER(3214, CL_BGRA);
> -        CHANNEL_ORDER(4123, CL_ARGB);

I'm not sure I believe this part:

1 = R
2 = G
3 = B
4 = A

gives

RGBA -> 1234
BGRA -> 3214
ARGB -> 4123
ABGR -> 4321

The others match, so why would ARGB be different?  2341 should be GBAR.

(Can you try this with multiple ARGB sources or OpenCL ICDs?  Maybe there is a bug somewhere else...)

>  #ifdef CL_ABGR
>          CHANNEL_ORDER(4321, CL_ABGR);
>  #endif
> 

Thanks,

- Mark
Ruiling Song April 9, 2019, 1:08 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of

> Mark Thompson

> Sent: Tuesday, April 9, 2019 3:49 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH v2] libavutil/hwcontext_opencl.c: fix bug in

> `opencl_get_plane_format`

> 

> On 08/04/2019 03:01, Jarek Samic wrote:

> > The `opencl_get_plane_format` function was incorrectly determining the

> > value used to set the image channel order. This resulted in all RGB

> > pixel formats being set to the `CL_RGBA` pixel format, regardless of

> > whether or not they actually *were* RGBA.

> >

> > This patch fixes the issue by using the `offset` and depth of components

> > rather than the loop index to determine the value of `order`.

> >

> > Signed-off-by: Jarek Samic <cldfire3@gmail.com>

> > ---

> > I have updated this patch in response to the comments on the first version.

> > RGB is no longer special-cased, the 2, 3, and 4 mappings to `CL_R` have been

> > removed, and the mapping for `CL_ARGB` has been changed to the correct

> value.

> >

> >  libavutil/hwcontext_opencl.c | 8 +++-----

> >  1 file changed, 3 insertions(+), 5 deletions(-)

> >

> > diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c

> > index b116c5b708..593de1ca41 100644

> > --- a/libavutil/hwcontext_opencl.c

> > +++ b/libavutil/hwcontext_opencl.c

> > @@ -1419,8 +1419,9 @@ static int opencl_get_plane_format(enum

> AVPixelFormat pixfmt,

> >          // from the same component.

> >          if (step && comp->step != step)

> >              return AVERROR(EINVAL);

> > -        order = order * 10 + c + 1;

> > +

> >          depth = comp->depth;

> > +        order = order * 10 + comp->offset / ((depth + 7) / 8) + 1;

> >          step  = comp->step;

> >          alpha = (desc->flags & AV_PIX_FMT_FLAG_ALPHA &&

> >                   c == desc->nb_components - 1);

> 

> This part LGTM, I can split it off and apply it on its own if you like.

> 

> > @@ -1456,14 +1457,11 @@ static int opencl_get_plane_format(enum

> AVPixelFormat pixfmt,

> >      case order: image_format->image_channel_order = type; break;

> >      switch (order) {

> >          CHANNEL_ORDER(1,    CL_R);

> > -        CHANNEL_ORDER(2,    CL_R);

> > -        CHANNEL_ORDER(3,    CL_R);

> > -        CHANNEL_ORDER(4,    CL_R);

> >          CHANNEL_ORDER(12,   CL_RG);

> >          CHANNEL_ORDER(23,   CL_RG);

> 

> 23 should be gone too, I think?

Agree.
> 

> >          CHANNEL_ORDER(1234, CL_RGBA);

> > +        CHANNEL_ORDER(2341, CL_ARGB);

> >          CHANNEL_ORDER(3214, CL_BGRA);

> > -        CHANNEL_ORDER(4123, CL_ARGB);

> 

> I'm not sure I believe this part:

> 

> 1 = R

> 2 = G

> 3 = B

> 4 = A

The above assumption is not true.
The new logic changes to use combination of offset-index of RGBA.
So for CL_ARGB, the R offset at 2, G is offset at 3, B is offset at 4, A is offset at 1.
So, it is 2341 that maps to ARGB.
It's interesting that these two ways of representing the swizzle sometime match, sometime not.

Thanks!
Ruiling
> 

> gives

> 

> RGBA -> 1234

> BGRA -> 3214

> ARGB -> 4123

> ABGR -> 4321

> 

> The others match, so why would ARGB be different?  2341 should be GBAR.

> 

> (Can you try this with multiple ARGB sources or OpenCL ICDs?  Maybe there is a

> bug somewhere else...)

> 

> >  #ifdef CL_ABGR

> >          CHANNEL_ORDER(4321, CL_ABGR);

> >  #endif

> >

> 

> Thanks,

> 

> - Mark

> _______________________________________________

> 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".
Mark Thompson April 9, 2019, 9:18 p.m.
On 09/04/2019 02:08, Song, Ruiling wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of
>> Mark Thompson
>> Sent: Tuesday, April 9, 2019 3:49 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v2] libavutil/hwcontext_opencl.c: fix bug in
>> `opencl_get_plane_format`
>>
>> On 08/04/2019 03:01, Jarek Samic wrote:
>>> The `opencl_get_plane_format` function was incorrectly determining the
>>> value used to set the image channel order. This resulted in all RGB
>>> pixel formats being set to the `CL_RGBA` pixel format, regardless of
>>> whether or not they actually *were* RGBA.
>>>
>>> This patch fixes the issue by using the `offset` and depth of components
>>> rather than the loop index to determine the value of `order`.
>>>
>>> Signed-off-by: Jarek Samic <cldfire3@gmail.com>
>>> ---
>>> I have updated this patch in response to the comments on the first version.
>>> RGB is no longer special-cased, the 2, 3, and 4 mappings to `CL_R` have been
>>> removed, and the mapping for `CL_ARGB` has been changed to the correct
>> value.
>>>
>>>  libavutil/hwcontext_opencl.c | 8 +++-----
>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c
>>> index b116c5b708..593de1ca41 100644
>>> --- a/libavutil/hwcontext_opencl.c
>>> +++ b/libavutil/hwcontext_opencl.c
>>> @@ -1419,8 +1419,9 @@ static int opencl_get_plane_format(enum
>> AVPixelFormat pixfmt,
>>>          // from the same component.
>>>          if (step && comp->step != step)
>>>              return AVERROR(EINVAL);
>>> -        order = order * 10 + c + 1;
>>> +
>>>          depth = comp->depth;
>>> +        order = order * 10 + comp->offset / ((depth + 7) / 8) + 1;
>>>          step  = comp->step;
>>>          alpha = (desc->flags & AV_PIX_FMT_FLAG_ALPHA &&
>>>                   c == desc->nb_components - 1);
>>
>> This part LGTM, I can split it off and apply it on its own if you like.
>>
>>> @@ -1456,14 +1457,11 @@ static int opencl_get_plane_format(enum
>> AVPixelFormat pixfmt,
>>>      case order: image_format->image_channel_order = type; break;
>>>      switch (order) {
>>>          CHANNEL_ORDER(1,    CL_R);
>>> -        CHANNEL_ORDER(2,    CL_R);
>>> -        CHANNEL_ORDER(3,    CL_R);
>>> -        CHANNEL_ORDER(4,    CL_R);
>>>          CHANNEL_ORDER(12,   CL_RG);
>>>          CHANNEL_ORDER(23,   CL_RG);
>>
>> 23 should be gone too, I think?
> Agree.
>>
>>>          CHANNEL_ORDER(1234, CL_RGBA);
>>> +        CHANNEL_ORDER(2341, CL_ARGB);
>>>          CHANNEL_ORDER(3214, CL_BGRA);
>>> -        CHANNEL_ORDER(4123, CL_ARGB);
>>
>> I'm not sure I believe this part:
>>
>> 1 = R
>> 2 = G
>> 3 = B
>> 4 = A
> The above assumption is not true.
> The new logic changes to use combination of offset-index of RGBA.
> So for CL_ARGB, the R offset at 2, G is offset at 3, B is offset at 4, A is offset at 1.
> So, it is 2341 that maps to ARGB.
> It's interesting that these two ways of representing the swizzle sometime match, sometime not.

Urgh, yes.  I was totally missing that the change above also changes the interpretation of the values.

With that I agree the values are correct, so LGTM.

Applied with a slightly more direct commit title.

Thanks!

- Mark

Patch hide | download patch | download mbox

diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c
index b116c5b708..593de1ca41 100644
--- a/libavutil/hwcontext_opencl.c
+++ b/libavutil/hwcontext_opencl.c
@@ -1419,8 +1419,9 @@  static int opencl_get_plane_format(enum AVPixelFormat pixfmt,
         // from the same component.
         if (step && comp->step != step)
             return AVERROR(EINVAL);
-        order = order * 10 + c + 1;
+
         depth = comp->depth;
+        order = order * 10 + comp->offset / ((depth + 7) / 8) + 1;
         step  = comp->step;
         alpha = (desc->flags & AV_PIX_FMT_FLAG_ALPHA &&
                  c == desc->nb_components - 1);
@@ -1456,14 +1457,11 @@  static int opencl_get_plane_format(enum AVPixelFormat pixfmt,
     case order: image_format->image_channel_order = type; break;
     switch (order) {
         CHANNEL_ORDER(1,    CL_R);
-        CHANNEL_ORDER(2,    CL_R);
-        CHANNEL_ORDER(3,    CL_R);
-        CHANNEL_ORDER(4,    CL_R);
         CHANNEL_ORDER(12,   CL_RG);
         CHANNEL_ORDER(23,   CL_RG);
         CHANNEL_ORDER(1234, CL_RGBA);
+        CHANNEL_ORDER(2341, CL_ARGB);
         CHANNEL_ORDER(3214, CL_BGRA);
-        CHANNEL_ORDER(4123, CL_ARGB);
 #ifdef CL_ABGR
         CHANNEL_ORDER(4321, CL_ABGR);
 #endif