[FFmpeg-devel] [PATCH 2/3] libavcodec: v4l2m2m: output AVDRMFrameDescriptor

[Dave Stevenson]

On 9 May 2018 at 00:33, Mark Thompson <sw at jkqxz.net> wrote:
> On 08/05/18 19:24, Lukas Rusak wrote:
>> +
>> +        layer->format = avbuf->context->av_pix_fmt == AV_PIX_FMT_NV12 ?
>> +            DRM_FORMAT_NV12 : DRM_FORMAT_NV21;
>> +        layer->nb_planes = 2;
>> +
>> +        layer->planes[1].object_index = 0;
>> +        layer->planes[1].offset = avbuf->plane_info[0].bytesperline *
>> +            avbuf->context->format.fmt.pix_mp.height;
>
> Is that always true?  I would expect that some driver might want more vertical alignment (especially with tiled formats) and would provide this number somewhere else.

The V4L2 spec defines their NV12/21 as:
"These are two-plane versions of the YUV 4:2:0 format. The three
components are separated into two sub-images or planes. The Y plane is
first. The Y plane has one byte per pixel. For V4L2_PIX_FMT_NV12, a
combined CbCr plane immediately follows the Y plane in memory. " [1]

Please be aware that there is now the V4L2 multi-planar API which
returns the planes in independent buffers (and an independent dma_buf
fd for each plane). That is not the case being handled here.
If being a real pedant, then it wants to be "layer->planes[1].offset =
avbuf->plane_info[0].bytesperline *
avbuf->context->format.fmt.pix.height;" as this should be using the
single planar API union member. width and height align between the two
structures anyway.

If you want the buffer to be a multiple of macroblocks or have other
padding requirements, then the width and height are defined to be that
size. Use the selection API [2] to get the cropping window.

>> +        layer->planes[1].pitch = avbuf->plane_info[0].bytesperline;
>> +        break;
>> +
>> +    case AV_PIX_FMT_YUV420P:
>> +
>> +        if (avbuf->num_planes > 1)
>> +            break;
>> +
>> +        layer->format = DRM_FORMAT_YUV420;
>> +        layer->nb_planes = 3;
>> +
>> +        layer->planes[1].object_index = 0;
>> +        layer->planes[1].offset = avbuf->plane_info[0].bytesperline *
>> +            avbuf->context->format.fmt.pix_mp.height;
>> +        layer->planes[1].pitch = avbuf->plane_info[0].bytesperline >> 1;
>> +
>> +        layer->planes[2].object_index = 0;
>> +        layer->planes[2].offset = layer->planes[1].offset +
>> +            ((avbuf->plane_info[0].bytesperline *
>> +              avbuf->context->format.fmt.pix_mp.height) >> 2);
>> +        layer->planes[2].pitch = avbuf->plane_info[0].bytesperline >> 1;
>
> Similarly here, and the pitch feels dubious as well.  Is plane_info[n].bytesperline set for n > 0?

Likewise the definition of YU12/YV12 is that the planes will follow
immediately [3]

Pass on plane_info[n] - I only know V4L2 in any depth.

>> +        break;
>> +
>> +    default:
>
> Probably want a more explicit failure in other cases.  Is there any 10-bit handling here yet (P010, I guess)?

Based on a quick search I don't believe V4L2 has added support for any
10 bit formats as yet, therefore it would be strange to get here with
a 10bit format selected.

  Dave

[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/pixfmt-nv12.html
[2] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/vidioc-g-selection.html#vidioc-g-selection
[3] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/pixfmt-yuv420.html