| Message ID | 20190408020123.6439-1-cldfire3@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
> -----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".
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
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
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(-)