Update and improve video capture sample

[ngphibang]

As mt9m114 and ov5640 camera shields have been merged. The sample needs to be updated too.
Some improvements on the video capture sample are also made:

• Display frames on LCD panel instead of just discarding them
• Add support for RT1170-evk and video sw generator

This PR is split from #69810 to ease the review process as required by @loicpoulain.
This PR depends on #72434 and #72435

[josuah]

Nice to see some complete device-to-device workflow.

This looks fairly similar to this pull request: #71463
Would it make more sense to split the "capture-only" or "capture to display" like this other pull request does?

[ngphibang]

Thanks, I am not aware of the PR you pointed out. BTW, this PR is part of the series #71463 which I did last year and first pushed the complete series on March 5, 2024.

About the "capture-only" vs "captue-to-display", I think

• It does not make much sense if we only capture frames and discard them. The original video capture sample normally should display the frame, but I think for some reasons, it was not done at that time.
• This PR itself already allows capture-only or capture to display depending on the availability of the hardwares, so we don't need 2 separate samples, I think.

[josuah]

Thank you for explaining, I was curious... The other sample uses LVGL: not the same strategy.

[mmahadevan108]

Has this been tested on 1064_evk after the updates to the sample?

[ngphibang]

@mmahadevan108 : yes, it's tested on 1064_evk, 1170_evk and evkb.

[kartben]

cc @CharlesDias

[ngphibang]

CI failures are not due to the PR changes.

[ngphibang]

• Updated the documentation to use a display shield for RT1064 because display stuffs WILL be removed from RT1064 EVK by this PR (which should be merged first although there is no direct dependency)

• Reorganisation of some commits.

@loicpoulain @danieldegrasse Could you revisit this ?

[josuah]

Glad to see all this past work making it all the way to the user-facing samples!

[josuah]

Good thing that this introduces alignment requirements: users might try the example and fail because alignment is required by their hardware. This gives a hint that this might need to be investigated.

Maybe some day in the future, alignment could be turned into a Kconfig option, so that each driver can check if the selected alignment value is high enough for their need...

[ngphibang]

yes, it's better with a Kconfig option in case that the hardware (e.g. PxP) really requires buffer alignment to work, and the driver uses it in code. But here, aligment is not mandatory (PxP driver does not use the aligment in code so I don't know how to define and check this Kconfig alignment option in PxP driver) though PxP performance could be improved if the buffer is (64B) aligned.

[ngphibang]

@josuah : Finally, I found and used the CONFIG_VIDEO_BUFFER_POOL_ALIGN which is available in the video subsystem (it is there but no one uses it :) ). This way, we can avoid platform specific stuffs in the code.

[josuah]

Great news! Putting it in the example also brings some context and visibility for it. Two birds one stone.

[josuah]

Is the #else really required? It looks like the device name is a built-time constant, so resolving the name at runtime with device_get_binding() is not required?
I am still unsure about which one to choose in which context.
An alternative would be adding an empty #else block with #error to hint the user about using a chosen block.

[ngphibang]

I think #else is required because if we don't have a real video hardware, the software generator is used, but if we have one, the video hardware is used, not both.

[josuah]

I thought the software generator could have been selected in the "chosen" block by default, and be modified/overrided by extra provided user app.overlay, or a snippet.

Having the fallback happen inline in the example makes sense too. I understand the rationale now. Thank you!

[danieldegrasse]

What is the reasoning behind setting the format here? Some displays will likely want to use a different format than the one this sample sets- it is really that we need the camera format and display format to match, right?

I suppose maybe what we need to do is only enable the display if the camera format is RGB565? I suppose we could also enable XRGB888 via the ARGB8888 display format

[ngphibang]

What is the reasoning behind setting the format here? Some displays will likely want to use a different format than the one this sample sets- it is really that we need the camera format and display format to match, right?

Yes, the camera and display formats must match. Normally, it is the (human) user who needs to look at both camera and display default formats (and capabilities) then sets the format. But here, we want the sample work "out-of-the-box", for RT1170 case, the camera output format is XRGB so we need to set the display (we have many displays in the world but here in this sample, it is eLCDIF & rk055hdmi4ma0) to this format.

I suppose maybe what we need to do is only enable the display if the camera format is RGB565?

I think it will be a big pity if we do that. The camera output on RT1170 is XRGB8888 and we cannot have camera frames displayed on LCD just because of the mismatch of the default formats between camera and display ? I know that you want to be generic but the sample supports RT1064 and RT1170 as reference boards, we can always set the display format to match these cases, no ? Also, through this, it will give some hints to people that we could "always" match the camera / display formats for their own camera / display set-up.

I suppose we could also enable XRGB888 via the ARGB8888 display format

Yes, XRGB is more generic than ARGB and we can use both alternatively. But because there is no PIXEL_FORMAT_XRGB_8888 defined in display.h, I used the other.

[danieldegrasse]

I think it will be a big pity if we do that. The camera output on RT1170 is XRGB8888 and we cannot have camera frames displayed on LCD just because of the mismatch of the default formats between camera and display ? I know that you want to be generic but the sample supports RT1064 and RT1170 as reference boards, we can always set the display format to match these cases, no ? Also, through this, it will give some hints to people that we could "always" match the camera / display formats for their own camera / display set-up.

I think I'm missing something here. I guess the PXP will convert the XRGB8888 data to RGB565 for the LCD? Ideally, the display pipeline would report its format at XRGB8888 in this case. From my understanding, the application either requires that the format of the buffer that is output from the video pipeline and input to the display pipeline be the same, or the application converts the buffer to a new format manually.

[ngphibang]

I think I'm missing something here. I guess the PXP will convert the XRGB8888 data to RGB565 for the LCD ?

No, as you can see, the current PxP driver does nothing apart from rotation. It's the eLCDIF itself who can support XRGB format (via this PR which was merged sometimes ago).

From my understanding, the application either requires that the format of the buffer that is output from the video pipeline and input to the display pipeline be the same,

I don't think so, as you can see, display controller (like eLCDIF) can support various formats, so application can set the display format at runtime, it does not require that the camera output format is the same as the default display format. It is the actual situation.

or the application converts the buffer to a new format manually.

Yes, this is another way. Application can use software or hardware like PxP to do so (we are working on a new PxP driver with more features).

[danieldegrasse]

No, as you can see, the current PxP driver does nothing apart from rotation. It's the eLCDIF itself who can support XRGB format (via #69250 which was merged sometimes ago).

Sure- sorry, I remembered seeing something for this, but wasn't sure where we added the support.

I don't think so, as you can see, display controller (like eLCDIF) can support various formats, so application can set the display format at runtime, it does not require that the camera output format is the same as the default display format. It is the actual situation.

Yes, I follow that. To be clear, what I am asking for here is the following code:

/* Set display pixel format to match the one in use by the camera */
switch (capabilities.current_pixel_format) {
case VIDEO_PIX_FMT_RGB565:
    display_set_pixel_format(display_dev, PIXEL_FORMAT_BGR_565);
case VIDEO_PIX_FMT_XRGB32:
    display_set_pixel_format(display_dev, PIXEL_FORMAT_ARGB_8888);
default:
    return -ENOTSUP;
}

My understanding is that the Sensor->CSI pipeline on the RT1064 would output VIDEO_PIX_FMT_RGB565 as the current format, and the Sensor->CSI2RX->CSI pipeline on the RT1170 would output VIDEO_PIX_FMT_XRGB32 as the current format. Is that incorrect?

[ngphibang]

Yes, this is definitely more elegant :). Thanks, done. FYI, I also removed BUFFER_ALIGN to avoid platform-specific stuffs as much as possible in the code. I used CONFIG_VIDEO_BUFFER_POOL_ALIGN which is available in the video subsystem.

[ngphibang]

CI twister test failure on RT1064 is because the zephyr,display chosen node is left and the node is disabled when display stuffs are already moved to a shield by this PR. I fixed this by this PR

[danieldegrasse]

One minor nit- otherwise lgtm. Going to approve given the time constraints of the merge window

[danieldegrasse]

This change should probably be in the prior commit, right?

[ngphibang]

I thought about this. I think both ways are ok. The previous commit adds support for display for the current supported board RT1064 which actually works without the need of this change because the default format of camera and display on RT1064 already matches. This change is needed essentially when we add support for RT1170 EVK.