yuv420 alignment problem

[glddiv]

Hello everyone,

Recently I ran into a problem. When I previewed the 800x650 yuv420 image (or stored the yuv420 data), I found that the color was shifted.
The effect is as follows:

After reviewing the code, I aligned the yuv420 in the code to 4, temporarily solving the preview problem:
https://github.com/raspberrypi/libcamera-apps/blob/d50ec797589b6aa057eb20854c12e806d63f8965/libcamera_app.hpp#L211

Any ideas? Should yuv420 be aligned to 4?

[6by9]

The alignment should support only being a multiple of 2 in either direction, but there's an error in the firmware calculation of where the V plane sits and it is aligning it to an address that is a multiple of 32 (instead of 16). This only generates a different answer under a few odd conditions, which is why it hadn't been spotted before.

I'm creating a firmware patch to resolve this.

[6by9]

Having just written that, there's a complication which requires a bit more thought.

Whilst the ISP will happily take a chroma stride that is only a multiple of 16, the start address for both chroma planes have to be on an address that is a multiple of 32. This causes this issue of the 2nd chroma plane start address being wrong when there are an odd number of lines in the 1st chroma plane, which equates to a luma height that is only a multiple of 2.

Use of any of the 2 plane YUV formats, or YUV422 (no vertical chroma subsampling), avoids the restrictions, but are more of a pain to handle elsewhere.

[glddiv]

Hi 6by9,

Thank you for your reply,
I'm not sure if I understand it correctly, but if the u and v planes need to be aligned to 32, it seems that 796x648 will also be wrong.
(796x648/4)%32=24
But this resolution seems to work properly.

Please correct me if I understand it wrong.

This is 796x648 yuv data, if you need it.
test.zip

[6by9]

Active area != buffer size.

I missed out the detail that the stride (number of bytes from the start of one line to the start of the next) of any buffer is rounded up to a multiple of 32. In V4L2 the stride of subsampled chroma planes is always the luma stride / subsample level.

So your 796 pixel wide line will work on a stride (V4L2 bytesperline) of 800 (796 active pixels/bytes and 4 padding bytes).
At 648 lines, your U plane will start at byte (800*648) = 518400 (hex 0x7e900).
The U plane is then 800/2 x 648/2 = 129600 (hex 0x1fa40) bytes in size. (398 active samples, and 2 padding bytes)
The V plane therefore starts at (0x7e900 + 0x1fa40) = 648000 (hex 0x9e340) which is 32 byte aligned.

In your original case of 800x650
Y starts at 0, size (800 * 650) 520000 (0x7ef40)
U starts at 520000 (0x7ef40), size (800/2 * 650/2) 130000 (0x1fbd0)
V starts at (520000 + 130000) 650000 (0x9eb10), size (800/2 * 650/2) 130000 (0x1fbd0)

That V start offset of 650000 (0x9eb10) is NOT 32byte aligned, so we get an issue in programming the ISP :-(

[naushir]

I'll look to see if it is feasible to set the Y plane alignment to 64 bytes for YUV420. This will correctly set the U/V plane alignments to 32. Will need a simultaneous change to kernel + firmware.

[6by9]

The simplest solution is to bump the alignment requirement for the luma bytesperline to 64 bytes, therefore the chroma planes will always be 32 byte aligned. It should be a simple case of changing the number at https://github.com/raspberrypi/linux/blob/rpi-5.10.y/drivers/staging/vc04_services/bcm2835-isp/bcm2835-isp-fmts.h#L51

[naushir]

The simplest solution is to bump the alignment requirement for the luma bytesperline to 64 bytes, therefore the chroma planes will always be 32 byte aligned. It should be a simple case of changing the number at https://github.com/raspberrypi/linux/blob/rpi-5.10.y/drivers/staging/vc04_services/bcm2835-isp/bcm2835-isp-fmts.h#L51

Does the firmware isp component know about the kernel defined stride? I was expecting to have to change the alignment there as well.

[6by9]

Once that's done, there's a similar alignment parameter in the firmware which is currently set as

      .input_min_alignment = {32, 1},

on YUV420PackedPlanar to stop non-V4L2 users of that component hitting the same issue.

[6by9]

Does the firmware isp component know about the kernel defined stride? I was expecting to have to change the alignment there as well.

It should get told it.
https://github.com/raspberrypi/linux/blob/rpi-5.10.y/drivers/staging/vc04_services/bcm2835-isp/bcm2835-v4l2-isp.c#L359

	port->es.video.width = (q_data->bytesperline << 3) / q_data->fmt->depth;
	port->es.video.height = q_data->height;
	port->es.video.crop.width = q_data->width;
	port->es.video.crop.height = q_data->height;

video.[width|height] is the buffer size.
video.crop.[width|height] is the active area of that buffer.
q_data->bytesperline should be aligned appropriately as part of try_fmt which applies that bytesperline_align alignment.

[naushir]

Great, I'll make the changes and give it a test.

[6by9]

If looking at doing the firmware change too, output_min_alignment and lowres_min_alignment both need to be increased too as the output formaters have the same restriction as the input formater (chroma stride is 16bit aligned, but chroma base addresses are 32bit).

[naushir]

Will do!

[naushir]

This should now be fixed with the latest kernel change.

@glddiv, please retest and close if it works for you.

[glddiv]

Hi 6by9
Thank you for your patient and detailed explanation.

Hi all
Great, I just tested the latest kernel and the problem has been solved.
Your work efficiency is impressive, thanks again!