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

[Tomasz Figa]

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.

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.

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