Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Video class #1118

Merged
merged 37 commits into from
Oct 12, 2021
Merged

Add Video class #1118

merged 37 commits into from
Oct 12, 2021

Conversation

kkitayam
Copy link
Collaborator

@kkitayam kkitayam commented Sep 29, 2021

Describe the PR
Add Video class driver and a video capture example. #908
This version is experimental and has many limitations. I have only implemented a minimum number of features.

  • Support only uncompressed format.
  • Support only isochronous IN. No support isochronous OUT and bulk.
  • Support only Full speed. So, the maximum bitrate is 1Mbps.
  • Support only input-terminal and output-terminal. No support any other unit.
  • No support dynamic streaming settings modification.
  • No support still image capture.

The video capture example runs as a virtual analog video capture device. The device captures TinyUSB logo animation panning color bars as 128x96@10fps NV12(YUV420 planar) YUY2. The video source is hard-coded into the flash.

Additional context
The video capture example was tested by using FRDM-KL25Z with VirtualDub for Windows.
CI reported some errors on other boards. I will resolve these errors to be ready for merge.

@hathach
Copy link
Owner

hathach commented Sep 30, 2021

AMAZING!!! brilliant PR 👍 👍 , thank you very much for making this. UVC along with UAC are 2 most complicated classes. This awesome PR with testable examples will greatly help further development (iso out, highspeed), and I have no doubt that we could pull this off together in the next several PRs.

Since the changes are huge, please give me a bit of time to digest it. I will also take the chance to test it with other ports as well. We will probably make an separated issue to keep track of which ports is tested with UCV (and UAC as well) similar to Compliance Verification in the future.

@kkitayam
Copy link
Collaborator Author

Thank you for your comments.
If you find any issues or questions, please let me know.

@kkitayam
Copy link
Collaborator Author

Fix CI errors, and I confirmed video capture example works fine on Raspberry Pi Pico and RX65N target board.

@hathach
Copy link
Owner

hathach commented Oct 11, 2021

@hathach

I have changed the video data to scrolling EBU color bars and stream format to YUY2 at b6d09ca.

Superb! I will do more tests with the new format (couldn't do in the weekend since there is lots of house work to be done 😄 )

@hathach
Copy link
Owner

hathach commented Oct 11, 2021

Tested and continue to work great with the color bar. I missed the logo however, the color bar shrink the size from 73KB to 24KB with either RAM or ROM via CFG_EXAMPLE_VIDEO_READONLY is a huge save. It will allow example to run on lots of smaller MCUs. Also the color bar generated code is short and straightforward, which can make it easier for user to do their own customization is a plus.

PS: still doing code review, will merge soon once complete. Only minor changes so far :)

minor code clean up without functional changes
Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

Brilliant work, this PR lays a great base work for more video improvement in the future. Along with UAC, UVC is one of the most complicated class driver. I have no doubt that this will absolutely help to spark lots of great application for lots of users. Thank you very much for this amazing work.

@hathach hathach merged commit 2ba690d into hathach:master Oct 12, 2021
@kkitayam kkitayam deleted the add_uvc branch October 13, 2021 11:49
@qwertymodo
Copy link

I've been playing around with this code, and have run into some issues with customizing it for my application. First of all, I'm unable to change the frame rate at all. Anything other than 10 (higher OR lower) causes Windows to no longer recognize it as a connected video device. Also, I'm trying to increase the resolution, and have hit a few walls there. I know that there are going to be limits wrt frame buffer size, due to RAM and time constraints, but I thought I'd post my findings here in case I'm missing something. I'm currently using the Raspberry Pi Pico, and if I increase the resolution above 128x128, the video device does show up, but all I get is a black screen, no color bars. I'm able to make it work if I also increase CFG_TUD_VIDEO_STREAMING_EP_BUFSIZE in tusb_config.h, which seems to max out at 1023 (if I set it to 1024 or anything higher, Windows no longer sees the video device). With the ep bufsize set to 1023, I can get the resolution up to about 256x190 before the color bars stop displaying and I just get a black screen again. Am I hitting hardware limitations here, or is there something else I can try tweaking to get the resolution up further?

@kkitayam
Copy link
Collaborator Author

@qwertymodo Thank you for your comments!

I'm able to make it work if I also increase CFG_TUD_VIDEO_STREAMING_EP_BUFSIZE in tusb_config.h, which seems to max out at 1023 (if I set it to 1024 or anything higher, Windows no longer sees the video device).

It is a limitation of the USB Full-Speed specification. Isochronous transfer of USB-FS up to 1023 bytes. Please find attached picture for USB 2.0 spec. Raspberry PI Pico supports only USB-FS in my understanding.
image

Current TinyUSB UVC class driver supports only isochronous transfer, but bulk transfer. The maximum bandwidth of the driver is 1 mega-bytes per second. It is a limitation for isochronous transfer of USB-FS.

With the ep bufsize set to 1023, I can get the resolution up to about 256x190 before the color bars stop displaying and I just get a black screen again. Am I hitting hardware limitations here, or is there something else I can try tweaking to get the resolution up further?

I think the settings reaches almost USB-FS limit. A 256x190 image needs 97,280 bytes(=256 x 190 x 16bits) per a frame in YUY2 raw pixel format. So, 256x190 @ 10 FPS consumes 972,800 bytes per second bandwidth.

To increase resolution and frame rate, We need to add other video formats reducing bandwidth consumption like as motion-jpeg.

@qwertymodo
Copy link

Good catch on the payload size limit, as well as the bandwidth discrepancy between isochronous and bulk transfers. It's been awhile since I did my last deep dive into the arcane lore that is the USB-IF documentation. Any ideas why I'm unable to change the framerate at all? Even lowering it to e.g. 8fps causes Windows to no longer recognize it as a video device.

@kkitayam
Copy link
Collaborator Author

Any ideas why I'm unable to change the framerate at all? Even lowering it to e.g. 8fps causes Windows to no longer recognize it as a video device.

I had tested with 10FPS and 5FPS. If you change to 5FPS instead of 8FPS, it works with Windows.

I have reproduced that video_capture example fails enumeration process with #define FRAME_RATE 8 in usb_descriptor.h
And USB Device Tree Viewer shows warning messages below.

*!*WARNING:  dwMaxFrameInterval minus dwMinFrameInterval is not evenly divisible by dwFrameIntervalStep, this could cause problems

I think that the following statements causes this warning.

(10000000/_fps), (10000000/_fps), 10000000, 100000), \

dwMinFrameInterval and dwFrameIntervalStep are calculated by 10000000/_fps. 10000000 is not evenly divisible by 8. Then the warning message occurs. So, in current video_capture implementation, FRAME_RATE must be a divisor of 10000000.
Of course, I think it is better to be able to specify any FPS. I will attempt to fix it later.

@qwertymodo
Copy link

dwMinFrameInterval and dwFrameIntervalStep are calculated by 10000000/_fps. 10000000 is not evenly divisible by 8. Then the warning message occurs. So, in current video_capture implementation, FRAME_RATE must be a divisor of 10000000. Of course, I think it is better to be able to specify any FPS. I will attempt to fix it later.

10000000 / 8 = 1250000, so it's not that, but from that warning it's probably something in the interaction between dwMaxFrameInterval, dwMinFrameInterval, and dwFrameIntervalStep

@qwertymodo
Copy link

Ok, I think I might have found the solution, but as I said, it's been awhile since I've read through the docs, so this is just a "seems to work" fix, without really being sure it's correct. I think the only change that needs to happen is that dwFrameIntervalStep needs to include the _fps factor. Changing this

(10000000/_fps), (10000000/_fps), 10000000, 100000), \

to

(10000000/_fps), (10000000/_fps), 10000000, (1000000/_fps)), \

seems to work. You may want to add or remove a zero or two in there, depending on the dwFrameIntervalStep granularity you actually want.

@kkitayam
Copy link
Collaborator Author

@qwertymodo Thank you for the solution! I have confirmed the issue is fixed.

I will create a PR for your solution with minor modification. It is shown below. I had confirmed it works with 6FPS and 8FPS.

(10000000/_fps), (10000000/_fps), (10000000/_fps)*_fps, (10000000/_fps)), \

@qwertymodo
Copy link

What's the point of the 3rd argument being divided by and then multiplied by _fps, doesn't that cancel out? Or does that achieve some kind of compile-time rounding?

@kkitayam
Copy link
Collaborator Author

What's the point of the 3rd argument being divided by and then multiplied by _fps, doesn't that cancel out? Or does that achieve some kind of compile-time rounding?

Yes, the value of (10000000/_fps)*_fps will be truncated to an integer.

When _fps is an integer, (10000000/_fps) is evaluated an integer and the decimal part of the result is truncated. So, when _fps is not a divisor of 10000000, (10000000/_fps)*_fps will be an integer which is not equal to 10000000.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants