MMAL: Discrepancy of I420 buffer lengths between components

[trya]

Using commit 6b855fb (compared to, say, 01ecfd2), length of I420 output buffers is a little bigger than expected at certain widths : for example, at 1440 pixels of width, even though it's a multiple of 32, the recommended buffer size is as if the supplied width was 1472 pixels.
This may cause subsequent components using such buffers not to function properly, as they do not expect buffers of that size.

Using this minimal use case that converts a RGB frame to I420 using the ISP and passes the resulting buffer to video_render: isp_output_issue.zip

• Before:

# vcgencmd version
Jan 22 2020 17:26:16 
Copyright (c) 2012 Broadcom
version 53a54c770c493957d99bf49762dfabc4eee00e45 (clean) (release) (start_x)

# isp_issue 1440 1080
vc.ril.video_render:in:0(I420): buf num 3 size 2350080
vc.ril.isp:out:0(I420): buf num 1 size 2350080
vc.ril.isp:in:0(RGB3): buf num 1 size 1327104
isp_out_cb: buf len 2350080

• After:

# vcgencmd version
Sep 28 2021 11:33:44 
Copyright (c) 2012 Broadcom
version 778b6a4f3c7d8d48bb63c02c47bcfbac79417bea (clean) (release) (start_x)

# isp_issue 1440 1080
vc.ril.video_render:in:0(I420): buf num 3 size 2350080
vc.ril.isp:out:0(I420): buf num 1 size 2402304
vc.ril.isp:in:0(RGB3): buf num 1 size 1327104
isp_out_cb: buf len 2350080
mmal: mmal_port_event_send: event lost on port 1,0 (buffer header callback not defined)

Allocation of a pool for the video_render input port was not necessary in order to demonstrate the discrepancy on the ISP side, but it illustrates what buffer size the component expects.

[6by9]

See raspberrypi/linux#4419

It's a requirement of the ISP hardware that the base address of all planes is a multiple of 32 on both input and output sides.
For 3-plane YUV420 that means that the luma needs to have a stride that is a multiple of 64 so that when subsampled the offset to the V plane (or U for YV21) is at an address that is a multiple of 32.

All components should accept a port format with a custom stride as long as it meets the other constraints of the port.

Your test app isn't checking the return value from mmal_port_format_commit. I would have expected that to have returned an error when you called it with a stride that wasn't a multiple of 64.

[trya]

Your test app isn't checking the return value from mmal_port_format_commit. I would have expected that to have returned an error when you called it with a stride that wasn't a multiple of 64.

I used assertions before uploading the test app, assert(mmal_port_format_commit(port) == MMAL_SUCCESS) didn't trigger, my userland is from commit raspberrypi/userland@97bc818 by the way.
So I get, that for a 1440-pixel wide YUV420 row, the stride should be VCOS_ALIGN_UP(1440*3/2, 64), that is, 2176 bytes instead of 2160 with no padding. But I don't see how I should pass a custom stride to format without calculating a proper width bigger than 1440 and giving a stride that is a multiple of 64. So, in the case of YUV420 encodings, width must be a multiple of 128:

format->es->video.width = (w%128==0) ? w : ((w/128)+1)*128;

In such case, my test program works with a width of 1440.

# ./isp_issue 1440 1080
vc.ril.video_render:in:0(I420): buf num 3 size 2506752
vc.ril.isp:out:0(I420): buf num 1 size 2506752
vc.ril.isp:in:0(RGB3): buf num 1 size 1327104
isp_out_cb: buf len 2506752

Not very convenient, but it does the job.

[6by9]

You appear to have an incorrect view of what stride sets for I420.
Stride or pitch is the number of bytes you need to skip ahead to drop down by one line. I420 is 3 planes one after another, with the stride defined as the stride of the luma plane, with the chroma planes having a stride of (luma stride / 2).

For your 1440 x 1080 image, you will get a stride of 1472 bytes (as a multiple of 64), and typically you'll get the height aligned up to a multiple of 16 (actually neither the ISP nor video_render enforce that requirement).
So it will be 1472x1088 bytes of Y/luma, then 736x544 bytes of U, then 736x544 bytes of V. A total of 2402304 bytes.

	format->es->video.width = VCOS_ALIGN_UP(w, 64);

is all you should need.

format->es->video.[width|height] defines the buffer geometry (ie stride/pitch and plane height)
format->es->video.crop.[width|height] defines the active area within that buffer.

You'll typically see

	port->format->es->video.crop.width = width;
	port->format->es->video.crop.height = height;
	port->format->es->video.width = VCOS_ALIGN_UP(port->format->es->video.crop.width, 64);
	port->format->es->video.height = VCOS_ALIGN_UP(port->format->es->video.crop.height, 16);

[trya]

Oh yes, I forgot that stride for I420 is not strictly the stride for one line of luma plane, that's where I got confused. So now I know that both ISP and video_render behave as expected with valid formats, I think my actual problem with buffer sizes comes from other components. I added image_fx inbetween isp and video_render and the discrepancy is there now:

# ./imgfx_issue 1440 1080
vc.ril.video_render:in:0(I420): buf num 3 size 2402304
vc.ril.image_fx:out:0(I420): buf num 1 size 2350080
vc.ril.image_fx:in:0(I420): buf num 1 size 2350080
vc.ril.isp:out:0(I420): buf num 1 size 2402304
vc.ril.isp:in:0(RGB3): buf num 1 size 1327104
isp_out_cb: buf len 2402304
mmal: mmal_port_event_send: event lost on port 1,0 (buffer header callback not defined)

Source: imgfx_output_issue.zip

Edit: this might also concern video_splitter, but not resize.

[6by9]

The original question has been answered. Are you happy to close?

[trya]

Not quite, the discrepancy remains with image_fx and video_splitter, even when padding buffer width to a multiple of 64. That's why I changed the title of issue.