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

[Song, Ruiling]

> -----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.

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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".