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

[Mark Thompson]

On 09/04/2019 02:08, Song, Ruiling wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
>> Mark Thompson
>> Sent: Tuesday, April 9, 2019 3:49 AM
>> To: ffmpeg-devel at 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 at 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