Re: [FFmpeg-devel] [PATCH v2] libavutil/hwcontext_opencl.c: fix bug in `opencl_get_plane_format`
On 09/04/2019 02:08, Song, Ruiling wrote:
>> -Original Message-
>> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@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
>>> ---
>>> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] libavutil/hwcontext_opencl.c: fix bug in `opencl_get_plane_format`
> -Original Message-
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@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
> > ---
> > 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-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
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
> ---
> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v2] libavutil/hwcontext_opencl.c: fix bug in `opencl_get_plane_format`
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
---
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);
@@ -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
--
2.21.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
