[Mesa-dev] [PATCH 2/2] egl/android: Use gralloc::lock_ycbcr for resolving YUV formats
Emil Velikov
emil.l.velikov at gmail.com
Thu Nov 10 15:17:36 UTC 2016
On 10 November 2016 at 07:13, Tomasz Figa <tfiga at chromium.org> wrote:
> 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.
>
Grr right - the chroma stride is always the same. Thanks
Emil
More information about the mesa-dev
mailing list