Skip to content

Conversation

ngphibang
Copy link
Contributor

  • Add a size field into video_format struct.
  • Video receiver drivers now need to set the format's size to expose to application. Application should base on the format size to allocate buffers. The caps' min/max_line_count fields (which are needed only for HWs that cannot support the whole image frame) can hence be dropped.

@avolmat-st
Copy link

It seems to me that currently there is a hole if a sensor provides a compressed format (ex: OV5640 which is capable, from a HW point of view, to provide JPEG data).
In such case, a receiver should not check base its calculation on the width, video_bits_per_pixel and height but rather directly on the size value provided by the source itself since, in case of compressed data there is just no way to figure out what could be size. The best entity to estimate this is the producer of the data, right ?

So, if the video_bits_per_pixel reports a non zero value, a receiver can estimate the pitch and size (as done in this PR), however if the video_bits_per_pixel is zero (as it would be in case of JPEG), then we cannot apply this calculation and need to get this from another way. (either from the sensor itself, since it could estimate it itself, knowing the quality settings etc etc, or by a rather simple estimation in each receiver, but not based on the video_bits_per_pixel).

@decsny decsny removed their request for review October 8, 2025 13:36
@ngphibang
Copy link
Contributor Author

ngphibang commented Oct 8, 2025

It seems to me that currently there is a hole if a sensor provides a compressed format (ex: OV5640 which is capable, from a HW point of view, to provide JPEG data)...

Yes, we should distinguish uncompressed vs compressed formats (can be based on fmt->pitch or video_bits_per_pixel).
For compressed formats, the size will be an estimate for the worse case as described in the fmt->field size. But the estimation will be added later when support for compressed image in sensors are added (e.g. ov5640).

Will update.

@avolmat-st
Copy link

It seems to me that currently there is a hole if a sensor provides a compressed format (ex: OV5640 which is capable, from a HW point of view, to provide JPEG data)...

Yes, we should distinguish uncompressed vs compressed formats (can be based on fmt->pitch or video_bits_per_pixel). For compressed formats, the size will be an estimate for the worse case as described in the fmt->field size. Will update.

Yes. Cannot be done on the pitch value I think since pitch is related to the way it is organized into memory, which is not applicable for sensors (which do not write themselves into memory). Hence my proposal to look at video_bits_per_pixel.
It is assume that sensor would not set the pitch. For uncompressed format, sensor do not need to set fmt.size, however for compressed format they will have to set fmt.size to give a worst case frame size.

Copy link
Contributor

@josuah josuah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/video/video_shell.c
This has a print statement for logging the max count

https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/video/video_emul_rx.c
This will be needing some conversion too, useful for testing this change on CI.

This sounds like a better solution than what I proposed here IMHO #92884 (comment)

/**
* @brief size of the buffer in bytes.
*
* For uncompressed formats, this is the size of the raw data buffer in bytes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any word about fractioned frames?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, reworded.

@zephyrbot zephyrbot requested a review from etienne-lms October 8, 2025 21:32
@ngphibang
Copy link
Contributor Author

Comments addressed. Note that, I didn't add the format size for compressed format because actually there is no in-tree supported for this case, so difficult to make an estimate. Such an estimate should be done in the future when jpeg format support is added.

josuah
josuah previously approved these changes Oct 9, 2025
@ngphibang ngphibang added the DNM This PR should not be merged (Do Not Merge) label Oct 9, 2025
@ngphibang
Copy link
Contributor Author

Sorry, I made some mistake with squashing commits somehow the change in video.h (dropping caps min/max_line_count) and in tcpserversink sample has gone. Just recovered it.

@ngphibang ngphibang removed the DNM This PR should not be merged (Do Not Merge) label Oct 9, 2025
@josuah
Copy link
Contributor

josuah commented Oct 9, 2025

tcpserversink sample has gone

I should have put a DNM flag on this PR #95862 as I did not find anything more to change so did +1 it, and it got merged before its dependencies (the current "add format size" PR).

Maybe this is why.

josuah
josuah previously approved these changes Oct 9, 2025
@josuah josuah requested a review from avolmat-st October 9, 2025 08:25
* The legacy pipe object API was removed. Use the new pipe API instead.
* ``bt_le_set_auto_conn``
* ``CONFIG_BT_BUF_ACL_RX_COUNT``
* Removed the ``min_line_count`` and ``max_line_count`` fields from :c:struct:`video_caps`.
Copy link

@avolmat-st avolmat-st Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add this in the migration guide ? If an out of tree app has been using those two fields, it won't compile anymore.

Copy link
Contributor Author

@ngphibang ngphibang Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. The release note is more visible than the migration note ? There is a section for removed stuffs in release note so I added it here. Or should add it in both docs ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migration guide should I think be here to deal with information users have to read in order to migrate from a previous version to the new one. Basically, without having to read the release-note is should be ok to use straight a new version after taking care of what is described in the migration-guide.
If a user is currently using those 2 fields, it won't build anymore so migration-guide should tell him things about that and optionaly indicates how it should be replaced (if necessary).

Can be added in both docs then I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Indeed API breakages should be documented in Migration Guide. See https://docs.zephyrproject.org/latest/develop/api/api_lifecycle.html.
Note also that release note redirects to Migratin Guide, so I'm not sure you need to duplicate the information unless the api breakage also introduce new feature and hence could be documented in release note as well.

Copy link
Contributor

@josuah josuah Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Something like this?

* Buffer size information obtained from :c:member:`video_caps.min_line_count` and
  :c:member:`video_caps.max_line_count` is now given by :c:member:`video_format.size`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, done. Indeed, I didn't notice that the changes in these fields affects also application, not only driver and for those changes, should go to migration guide:

This document describes the changes required when migrating your application from Zephyr v4.2.0 to
Zephyr v4.3.0.

Any other changes (not directly related to migrating applications) can be found in
the :ref:release notes<zephyr_4.3>.

Copy link

@avolmat-st avolmat-st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small point regarding the migration-guide

@josuah josuah requested a review from avolmat-st October 9, 2025 09:26
@josuah
Copy link
Contributor

josuah commented Oct 9, 2025

Small point regarding the migration-guide

My bad, I just saw this now... Nevermind the review request.

avolmat-st
avolmat-st previously approved these changes Oct 9, 2025
Copy link

@avolmat-st avolmat-st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ngphibang. All good to me.

return video_get_caps(config->source_dev, caps);
}

static void video_esp32_compute_fmt(struct video_format *fmt)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about making this a common function / helper instead of having this in each driver, and also do the estimation based on the pixelformat ?
So, the proposal is to have a helper that process the size / pitch and basically all drivers call the helper to set their pitch / size.
That should work for most of the drivers and if a driver has a specific pitch, then it won't use the helper and do it within the driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, done. A helper would be better.

Add a helper to estimate format size and pitch.

Signed-off-by: Phi Bang Nguyen <[email protected]>
Receiver drivers now need to set the format's size to expose it to
the application.

Application should base on the format size to allocate buffers. The
caps' min/max_line_count (which are needed only for HWs that cannot
support the whole image frame) can hence be dropped.

Signed-off-by: Phi Bang Nguyen <[email protected]>
The mipid02 is just a bridge driver which does not deal with memory so
should not expose caps' min_vbuf_count.

Signed-off-by: Phi Bang Nguyen <[email protected]>
Mention some dropped fields in the video_caps structure and the buffer
allocation size for application.

Signed-off-by: Phi Bang Nguyen <[email protected]>
Mention a new video_estimate_fmt_size helper.

Signed-off-by: Phi Bang Nguyen <[email protected]>
Copy link

sonarqubecloud bot commented Oct 9, 2025

Please retry analysis of this Pull-Request directly on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Samples Samples area: USB Universal Serial Bus area: Video Video subsystem platform: ESP32 Espressif ESP32 platform: NXP NXP platform: Renesas RA Renesas Electronics Corporation, RA platform: STM32 ST Micro STM32 Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants