GNOME Bugzilla – Bug 755937
v4l2object: probe colorspace supported by device
Last modified: 2016-03-25 14:56:46 UTC
Currently we only add in caps the default colorimetry info given by the device for a specific video dimension and pixelformat. However a device can support more than one colorspace and so we could end up with a negotiation failure if the colorimetry doesn't match even if the v4l2 device can support the video colorspace. This issue has been introduce 7a1613b9e15ab3e437c56b4de7bc15a30ea4f619. I discovered this issue when trying to play 'Sintel' on my platform. The h264parser set the colorimetry in caps to bt601, but my v4l2sink default colorimetry was bt709, thus leading to a negotiation error.
Created attachment 312475 [details] [review] v4l2object: probe all colorspace supported by device Proposed patch to fix the issue. It currently probe all the colorspace, so it slightly delay the v4l2 initialization: on my board (Cortex-A9), gst_v4l2_object_add_colorspace now takes around 13 ms instead of 50 us without probing. However, I think there is some rooms for improvements and in the future, we may discuss with V4L2 maintainers to see if the V4L2 api could have a way to enumerate colorspace like in ENUM_FMT or with other ioctl.
Review of attachment 312475 [details] [review]: Outch, that seems a little expensive, but indeed is the only way for now. We need a way to enumerate colorspace in v4l2 I guess. The code looks correct to me, so I don't see why we should wait. Can that be tested against vivid driver, and if so, do I need special setup ?
(In reply to Nicolas Dufresne (stormer) from comment #2) > Review of attachment 312475 [details] [review] [review]: > > Outch, that seems a little expensive, but indeed is the only way for now. We > need a way to enumerate colorspace in v4l2 I guess. The code looks correct > to me, so I don't see why we should wait. Can that be tested against vivid > driver, and if so, do I need special setup ? I don't remember if we could play with colorspace with vivid, just let me recompile it and check it.
It could be tested with Vivid on a 4.2 kernel :) To do it, I loaded vivid module with following parameters "num_outputs=1 output_types=0x1" which make vivid create an output device with only one output simulating an HDMI output. You need this since we don't support output selection yet. So using the following pipeline: gst-launch-1.0 videotestsrc ! video/x-raw,colorimetry=bt601 ! v4l2sink without the patch, it doesn't work since probed caps only contains bt709 With the patch, it works and the probed caps contains colorimetry=(string){ bt709, bt601, 1:4:7:1, bt2020 }
That's really cool, I'll give that a try too. Reason I was asking, is that I was considering starting this phase with adding unit test using that driver. Ideally if can can manager without setting modules params (just the defaults) that would be better. Though, I'm not sure we can enable mplane and single planar at the same time.
Btw, is the opposite properly working, does: v4lsrc ! video/x-raw,colorimetry=bt2020 ! .. Will negotiate ?
Hmm, can't merge this, with UVC driver and this patch, probing seems to take forever.
gst-launch-1.0 v4l2src device=/dev/video0 num-buffers=1 ! fakesink Before: real 0m1.186s After real 1m28.960s That's more then "slightly" I must say.
(In reply to Nicolas Dufresne (stormer) from comment #6) > Btw, is the opposite properly working, does: > > v4lsrc ! video/x-raw,colorimetry=bt2020 ! .. > > Will negotiate ? With Vivid, no. Accordin to the v4l2 spec colorspace is set by the driver for a capture device and vivid seems to not allow variation. But this definition could be annoying for colorspace converter. (In reply to Nicolas Dufresne (stormer) from comment #8) > gst-launch-1.0 v4l2src device=/dev/video0 num-buffers=1 ! fakesink > > Before: > real 0m1.186s > After > real 1m28.960s > > That's more then "slightly" I must say. Agreed. Well I think ioctl flood on USB interface is not really efficient :-P. For now I think of two solutions: first we can only probe output devices since, as I said above, there is no room in the spec for the application to set the colorspace for capture. A second one is to just probe the colorspace and not all the colorspace/matrix/transfer/range variants. These two changes can also be done together. I will update the patch tomorrow.
Created attachment 312550 [details] [review] v4l2object: probe all colorspace supported by device Lighter patch which only check colorspace, not all combination of colorspace/matrix/range/transfer. However, it should cover common use cases. It reduces the ioctl from 1620 to 10 so it should speed-up with UVC camera. Nicolas, could you test it with UVC camera please ?
This will need to be rebase on master, since we have made some changes to the colorimetry recently.
Created attachment 319742 [details] [review] v4l2object: probe all colorspace supported by device Rebased on master with the addition of cosmetic matrix substitution for RGB formats.
Created attachment 324681 [details] [review] v4l2object: probe all colorspace supported by device Rebase on current master and restore a comment which was removed in last rebase.
Performance is good enough (nearly un-notice).
Attachment 324681 [details] pushed as c163250 - v4l2object: probe all colorspace supported by device
(In reply to Nicolas Dufresne (stormer) from comment #14) > Performance is good enough (nearly un-notice). Good to hear that. Thanks for review and merge.
Should this go into 1.8?
(In reply to Sebastian Dröge (slomo) from comment #17) > Should this go into 1.8? I would still give it some time as it may have greater performance impact then I am currently testing. I'd say if Aurélien or someone else request it. This remains a fairly rare use case.
(In reply to Nicolas Dufresne (stormer) from comment #18) > (In reply to Sebastian Dröge (slomo) from comment #17) > > Should this go into 1.8? > > I would still give it some time as it may have greater performance impact > then I am currently testing. I'd say if Aurélien or someone else request it. > This remains a fairly rare use case. Agreed. I think, we can let it in master for now to see if there is a performance issue.