[Mesa-dev] [PATCH 2/2] egl/android: Use gralloc::lock_ycbcr for resolving YUV formats

Tomasz Figa tfiga at chromium.org
Thu Nov 10 07:13:36 UTC 2016


On Thu, Nov 10, 2016 at 1:50 PM, Tomasz Figa <tfiga at chromium.org> wrote:
> On Thu, Nov 10, 2016 at 5:14 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 9 November 2016 at 08:33, Tomasz Figa <tfiga at chromium.org> wrote:
>>> There is an interface that can be used to query YUV buffers for their
>>> internal format. Specifically, if gralloc:lock_ycbcr() is given no SW
>>> usage flags, it's supposed to return plane offsets instead of pointers.
>>> Let's use this interface to implement support for YUV formats in Android
>>> EGL backend.
>>>
>> gralloc:lock_ycbcr seems to be missing from gbm_gralloc.
>> Seems trivial to implement - one can even apply the drm_gralloc commit
>> a43d9523c050b3ddb40190bb8de7920b7521b6ca directly ;-)
>>
>>> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
>>> ---
>>>  src/egl/drivers/dri2/platform_android.c | 146 ++++++++++++++++++++++++++------
>>>  1 file changed, 120 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
>>> index 0b59724..696a22f 100644
>>> --- a/src/egl/drivers/dri2/platform_android.c
>>> +++ b/src/egl/drivers/dri2/platform_android.c
>>> @@ -43,6 +43,49 @@
>>>
>>>  #define ALIGN(val, align)      (((val) + (align) - 1) & ~((align) - 1))
>>>
>>> +struct droid_yuv_format {
>>> +   /* Lookup keys */
>>> +   int native;
>>> +   int is_ycrcb;
>>> +   int chroma_step;
>>> +
>>> +   /* Result */
>>> +   int fourcc;
>>> +};
>>> +
>>> +static const struct droid_yuv_format droid_yuv_formats[] = {
>>> +   { HAL_PIXEL_FORMAT_YCbCr_420_888, 0, 2, __DRI_IMAGE_FOURCC_NV12 },
>>> +   { HAL_PIXEL_FORMAT_YCbCr_420_888, 0, 1, __DRI_IMAGE_FOURCC_YUV420 },
>>> +   { HAL_PIXEL_FORMAT_YCbCr_420_888, 1, 1, __DRI_IMAGE_FOURCC_YVU420 },
>>> +   { HAL_PIXEL_FORMAT_YV12, 1, 1, __DRI_IMAGE_FOURCC_YVU420 },
>>> +};
>>> +
>> This table here feels a bit magical. Please add a comment explaining things.
>>
>>> +static int
>>> +get_fourcc_yuv(int native, int is_ycrcb, int chroma_step)
>>> +{
>>> +   int i;
>>> +
>>> +   for (i = 0; i < ARRAY_SIZE(droid_yuv_formats); ++i)
>>> +      if (droid_yuv_formats[i].native == native &&
>>> +          droid_yuv_formats[i].is_ycrcb == is_ycrcb &&
>>> +          droid_yuv_formats[i].chroma_step == chroma_step)
>>> +         return droid_yuv_formats[i].fourcc;
>>> +
>>> +   return 0;
>> get_fourcc() uses -1 for error/not-found. Can we have both functions
>> behave identically - don't mind each way.
>>
>>> +}
>>> +
>>> +static int
>> We haven't used it too often - yet "bool" would be nice.
>>
>>> +is_yuv(int native)
>>> +{
>>> +   int i;
>>> +
>>> +   for (i = 0; i < ARRAY_SIZE(droid_yuv_formats); ++i)
>>> +      if (droid_yuv_formats[i].native == native)
>>> +         return 1;
>>> +
>>> +   return 0;
>>> +}
>>> +
>>>  static int
>>>  get_format_bpp(int native)
>>>  {
>>> @@ -57,9 +100,6 @@ get_format_bpp(int native)
>>>     case HAL_PIXEL_FORMAT_RGB_565:
>>>        bpp = 2;
>>>        break;
>>> -   case HAL_PIXEL_FORMAT_YV12:
>>> -      bpp = 1;
>>> -      break;
>>>     default:
>>>        bpp = 0;
>>>        break;
>>> @@ -76,7 +116,6 @@ static int get_fourcc(int native)
>>>     case HAL_PIXEL_FORMAT_BGRA_8888: return __DRI_IMAGE_FOURCC_ARGB8888;
>>>     case HAL_PIXEL_FORMAT_RGBA_8888: return __DRI_IMAGE_FOURCC_ABGR8888;
>>>     case HAL_PIXEL_FORMAT_RGBX_8888: return __DRI_IMAGE_FOURCC_XBGR8888;
>>> -   case HAL_PIXEL_FORMAT_YV12:      return __DRI_IMAGE_FOURCC_YVU420;
>>>     default:
>>>        _eglLog(_EGL_WARNING, "unsupported native buffer format 0x%x", native);
>>>     }
>>> @@ -479,35 +518,68 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw)
>>>  }
>>>
>>>  static _EGLImage *
>>> -droid_create_image_from_prime_fd(_EGLDisplay *disp, _EGLContext *ctx,
>>> -                                 struct ANativeWindowBuffer *buf, int fd)
>>> +droid_create_image_from_prime_fd_yuv(_EGLDisplay *disp, _EGLContext *ctx,
>>> +                                     struct ANativeWindowBuffer *buf, int fd)
>>>  {
>>> -   unsigned int offsets[3] = { 0, 0, 0 };
>>> -   unsigned int pitches[3] = { 0, 0, 0 };
>>> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>>> +   struct android_ycbcr ycbcr;
>>> +   size_t offsets[3];
>>> +   size_t pitches[3];
>>> +   int is_ycrcb;
>>> +   int fourcc;
>>> +   int ret;
>>>
>>> -   const int fourcc = get_fourcc(buf->format);
>>> -   if (fourcc == -1) {
>>> -      _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR");
>>> +   if (!dri2_dpy->gralloc->lock_ycbcr) {
>>> +      _eglLog(_EGL_WARNING, "Gralloc does not support lock_ycbcr");
>>>        return NULL;
>>>     }
>>>
>>> -   pitches[0] = buf->stride * get_format_bpp(buf->format);
>>> -   if (pitches[0] == 0) {
>>> -      _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR");
>>> +   memset(&ycbcr, 0, sizeof(ycbcr));
>>> +   ret = dri2_dpy->gralloc->lock_ycbcr(dri2_dpy->gralloc, buf->handle,
>>> +                                       0, 0, 0, 0, 0, &ycbcr);
>>> +   if (ret) {
>>> +      _eglLog(_EGL_WARNING, "gralloc->lock_ycbcr failed: %d", ret);
>>>        return NULL;
>>>     }
>>> +   dri2_dpy->gralloc->unlock(dri2_dpy->gralloc, buf->handle);
>>> +
>>> +   offsets[0] = (size_t)ycbcr.y;
>>> +   is_ycrcb = (size_t)ycbcr.cb < (size_t)ycbcr.cr;
>>> +   if (is_ycrcb) {
>>> +      offsets[1] = (size_t)ycbcr.cr;
>>> +      offsets[2] = (size_t)ycbcr.cb;
>>> +   } else {
>>> +      offsets[1] = (size_t)ycbcr.cb;
>>> +      offsets[2] = (size_t)ycbcr.cr;
>>> +   }
>>>
>>> -   switch (buf->format) {
>>> -   case HAL_PIXEL_FORMAT_YV12:
>>> -      /* Y plane is assumed to be at offset 0. */
>>> -      /* Cr plane is located after Y plane */
>>> -      offsets[1] = offsets[0] + pitches[0] * buf->height;
>>> -      pitches[1] = ALIGN(pitches[0] / 2, 16);
>>> -      /* Cb plane is located after Cr plane */
>>> -      offsets[2] = offsets[1] + pitches[1] * buf->height / 2;
>>> -      pitches[2] = pitches[1];
>>> +   pitches[0] = ycbcr.ystride;
>>> +   pitches[1] = pitches[2] = ycbcr.cstride;
>>>
>> Going through the system/graphics.h description
>>   @cstride is the stride of the chroma planes
>
> There are also few lines above:
>
>  * Stride describes the distance in bytes from the first value of one row of
>  * the image to the first value of the next row.  It includes the width of the
>  * image plus padding.
>
>>
>> I'd imagine that one should interpret it as:
>>   @cstride is a) the total stride of the chroma planes when
>> interleaved or b) the individual stride of either chroma plane when
>> planar.
>
> Yeah, that's sounds right.
>
>>
>> Doesn't Android support planar YUV 4:2:2/YUV 4:1:1 formats ? If it
>> only does HAL_PIXEL_FORMAT_YV12 where the Cr Cb strides are the same
>> that would explain "either chroma plane" above.

Actually, do we really have any format where Cb and Cr planes are
different? The formats I've worked with always had symmetric Cb and Cr
planes, i.e. cstride == cbstride == crstride. This holds true even for
interleaved formats, since the distance between first pixels of two
subsequent lines is always the same, regardless of whether we refer to
the Cr or Cb byte.

>
> It supports other subsampling ratios, but only 4:2:0 is required from
> the gralloc. With HAL_PIXEL_FORMAT_YCbCr_420_888, the only thing
> specified is that the subsampling is 4:2:0 and each pixel occupies 8
> bits. The exact format specification has to be obtained by other
> means, such as the lock_ycbcr, as this patch does. This in practice
> can translate to NV12, NV21, YUV420 or YVU420. Although possibly the
> struct could also be used to describe fully interleaved YUYV and
> friends (possibly chroma_step 4 and the offsets being within 4 bytes?)
>
>> Can we add a comment about what cstride is and how it works in this case ?
>
> Sure.

Given the above, I'm not sure anymore what I should write in that
comment. Could you suggest something?

>
>>
>> Mildly related: while skimming through hardware/gralloc.h I've noticed
>> a duplicate hardware/hardware.h include ;-)
>
> ;-)
>
>>
>> Considering I understood things correctly - with the above trivial
>> suggestions the patch is
>> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>
> Thanks.
>
> Best regards,
> Tomasz


More information about the mesa-dev mailing list