media/capture: Add support for V4L2 MPLANE devices [chromium/src : master]
Daniel Mack (Gerrit)
This change is ready for review.
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Mack (Gerrit)
Could this be reviewed before it doesn't apply anymore, please?
Christian Fremerey (Gerrit)
Sorry for the inactivity. You haven't added anyone as reviewer yet, which is probably why it fell off the radar.
I am not an expert with the V4L2 details, but I can take a look to help review this.
Daniel Mack (Gerrit)
Patch Set 1:
Sorry for the inactivity. You haven't added anyone as reviewer yet, which is probably why it fell off the radar.
I am not an expert with the V4L2 details, but I can take a look to help review this.
No problem, I just wanted to trigger someone :)
I didn't add any reviewer because I have no idea who would be suitable. I have no other involvement in the Chromium project at this point and would have hoped this is dispatched to and handled by the respective teams.
If I can do anything else, please let me know.
Christian Fremerey (Gerrit)
Patch Set 1:
Patch Set 1:
Sorry for the inactivity. You haven't added anyone as reviewer yet, which is probably why it fell off the radar.
I am not an expert with the V4L2 details, but I can take a look to help review this.
No problem, I just wanted to trigger someone :)
I didn't add any reviewer because I have no idea who would be suitable. I have no other involvement in the Chromium project at this point and would have hoped this is dispatched to and handled by the respective teams.
If I can do anything else, please let me know.
Makes sense. No worries.
The way it typically works is that until a reviewer is assigned, it is assumed that the CL is not yet ready for review and the uploader is still working on it.
In order to find reviewers, you can look at the OWNER files in the folders (or parent folders) of the files you touched. Since I am an OWNER of media/capture, you can add me as reviewer and click Start Review.
Daniel Mack (Gerrit)
Daniel Mack would like Christian Fremerey, Tommi, Emircan Uysaler and Miguel Casas to review this change.
media/capture: Add support for V4L2 MPLANE devices
The Linux V4L2 API knows different types of video devices. Single planar
ones are the most common and are used by all USB cameras, while multiplanar
types are used by the drivers of some embedded chipsets such as Qualcomm
Snapdragon.
Support for multiplanar cameras was once introduced to the code base
(Issue 441836) but then removed again, most probably accidentially (Issue
585519).
This patch brings back support for these types of devices but in a simpler
way of implementation than the original approach, as all the device
specific code is kept internal to the V4L2CaptureDelegate class.
Some static inline functions were turned into class members in order to
keep the number of parameters low.
Bug: 768887
media/capture: register new type LINUX_V4L2_MULTI_PLANE
This new type will be used for multiplanar video devices exposed by the V4L2
Linux kernel API.
[For the lack of multiplanar hardware, this change set was only tested for
regressions with single plane cameras on x86_64. An equivalent set was used
against version 56, which is the version currently bundled for Qt's
QWebEngine 5.9 to confirm the functionality of multi-planar video devices
on arm64.]
Change-Id: I094ebf03b9ecf092911eb229d20267540b74e1e4
---
M media/capture/mojo/video_capture_types.mojom
M media/capture/mojo/video_capture_types_typemap_traits.cc
M media/capture/video/linux/v4l2_capture_delegate.cc
M media/capture/video/linux/v4l2_capture_delegate.h
M media/capture/video/linux/video_capture_device_factory_linux.cc
M media/capture/video/video_capture_device_descriptor.cc
M media/capture/video/video_capture_device_descriptor.h
7 files changed, 184 insertions(+), 73 deletions(-)
Daniel Mack (Gerrit)
In order to find reviewers, you can look at the OWNER files in the folders (or parent folders) of the files you touched. Since I am an OWNER of media/capture, you can add me as reviewer and click Start Review.
Okay, got it, thanks a bunch!
Christian Fremerey (Gerrit)
Patch Set 1:
In order to find reviewers, you can look at the OWNER files in the folders (or parent folders) of the files you touched. Since I am an OWNER of media/capture, you can add me as reviewer and click Start Review.
Okay, got it, thanks a bunch!
Hehe, sorry I should have mentioned that it is not necessary to add all of the owners. Typically it is enough to start with 1 reviewer and add more later only as needed.
I can do the first review round and if questions arise we can ask the others to take a look at well.
Daniel Mack (Gerrit)
Daniel Mack removed Tommi from this change.
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Mack (Gerrit)
Patch Set 1:
Patch Set 1:
In order to find reviewers, you can look at the OWNER files in the folders (or parent folders) of the files you touched. Since I am an OWNER of media/capture, you can add me as reviewer and click Start Review.
Okay, got it, thanks a bunch!
Hehe, sorry I should have mentioned that it is not necessary to add all of the owners. Typically it is enough to start with 1 reviewer and add more later only as needed.
I can do the first review round and if questions arise we can ask the others to take a look at well.
Okay, sorry. Let's do that then. Thanks for bearing with me :)
[I only saw now that the patch already has a merge conflict. I'm not currently on the computer I did this work on. I'll have access to it in about a month from now.]
Daniel Mack (Gerrit)
Daniel Mack removed Emircan Uysaler from this change.
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Mack (Gerrit)
Daniel Mack removed Miguel Casas from this change.
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Patch Set 1:
Patch Set 1:
Patch Set 1:
In order to find reviewers, you can look at the OWNER files in the folders (or parent folders) of the files you touched. Since I am an OWNER of media/capture, you can add me as reviewer and click Start Review.
Okay, got it, thanks a bunch!
Hehe, sorry I should have mentioned that it is not necessary to add all of the owners. Typically it is enough to start with 1 reviewer and add more later only as needed.
I can do the first review round and if questions arise we can ask the others to take a look at well.
Okay, sorry. Let's do that then. Thanks for bearing with me :)
[I only saw now that the patch already has a merge conflict. I'm not currently on the computer I did this work on. I'll have access to it in about a month from now.]
No problem. If you have a different computer where you can check out Chromium, you can simply download the changes from this code. Just click Download on the right side above the list of files and it presents several options for how to get/apply the change to your local checkout.
Miguel Casas (Gerrit)
1 comment:
File media/capture/video/video_capture_device_descriptor.h:
Patch Set #1, Line 21:
LINUX_V4L2_MULTI_PLANEThe original code was reverted not by mistake but because
there were no devices in use with this planarity - so this
was tantamount to dead code. Back in the day also the
linuxtv.org test code (i.e. vivi and the likes) didn't
properly cover multiplanar YUV formats. So, before landing
code we need make sure that it's properly used and tested;
I see you found some Qualcomm cameras using this format, so
that's good -- but somehow we need to make sure that this
code is tested on the trybots and, if possible, in the CQ
(via unittests and also via some kind of OS emulator).
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
1 comment:
Patch Set #1, Line 21:
LINUX_V4L2_MULTI_PLANE
The original code was reverted not by mistake but because […]
I agree with mcasas@ that we need this to be covered by automated tests.
In order to test this without requiring actual camera hardware or OS emulators, one possibility I see would be to mock out all of the V4L2 API calls and then configure the mock to behave like we expect V4L2 to behave on real hardware.
mcasas@: Would you see this as an acceptable option?If we need coverage on real hardware, we could check if there is a way to add a webrtc bot running on a device using one of these Qualcomm chips.
zonque@: Could you share more details on particular devices that could be used for such tests?
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Mack (Gerrit)
I've now rebased the two patches onto the current master. I'd very much like to get this reviewed.
The comment from last round still applies though, as the code supporting this camera type is not currently unit-tested. As I said, I don't know how to do that in the Chromium project. However, given that such tests are being made a requirement for this patch set, I assume there is some mock code that tests the SPLANE cameras, and that code should be easily extendible for MPLANE types as well.
1 comment:
Patch Set #1, Line 21:
LINUX_V4L2_MULTI_PLANEThe original code was reverted not by mistake but because […]
The change set that removed that support didn't mention any of that reasoning, or did I miss anything?
As my change set says, this PR is a forward port of a patch that I did for version 56, because that's the version that is currently bundled in Qt5.9, and that's the thing we're using on the Qualcomm processor. Last time I checked, the latest version of Chromium could not be built on arm64, but maybe that has changed now. If that's the case, I can test Chromium directly again and rebase my changes.
However, how to bring this into your GQ system beats me, that has to be done by someone else.
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Mack (Gerrit)
Patch Set 2:
(1 comment)
I've now rebased the two patches onto the current master. I'd very much like to get this reviewed.
The comment from last round still applies though, as the code supporting this camera type is not currently unit-tested. As I said, I don't know how to do that in the Chromium project. However, given that such tests are being made a requirement for this patch set, I assume there is some mock code that tests the SPLANE cameras, and that code should be easily extendible for MPLANE types as well.
To give some more background on this patch set: we are building embedded hardware based on Qualcomm's MSM8016 (aka Snapdragon 410), and we're using web technology for the UI components. For this, we chose QWebEngine, which is based on Chromium 56 in Qt 5.9. As we need to support the cameras in our systems in the browser views, I had to come up with a downstream patch for MPLANE types which works very well since a while. The planarity type is not enforced by the camera modules in our designs but by the Qualcomm camera subsystem in the mainline Linux kernel.
The purpose of the effort to merge this patch upstream is that I don't want to rebase my patch all the time when Qt updates their Chromium version. I also believe that more people are affected by the current limitation to SPLANE cameras. Let me know if I can provide any other information.
Alexandre Courbot (Gerrit)
2 comments:
File media/capture/video/linux/v4l2_capture_delegate.cc:
Patch Set #2, Line 340:
if (isMultiplane())Contrary to the one in Init(), this call to isMultiplane() can be moved out of the loop without duplicating code.
Patch Set #2, Line 958:
if (isMultiplane()) {I feel like this isMultiPlane() (and the one in set_payload_size()) should be outside of the loop, since there will only be one plane if it is false anyway. In this case multiple iterations should not happen and would be an error. But I also understand that doing so would duplicate the mmap() code. Maybe perform a runtime check on (isMultiplane() || num_planes_ == 1) and trigger an error if this condition is not met to ensure consistency?
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Mack (Gerrit)
Patch Set 2:
(2 comments)
Thanks for the comments. As commented inline, I agree with the change in line 340, but I'm sceptical about the other one. I can repost a new set with just the one change if you want.
2 comments:
Patch Set #2, Line 340:
if (isMultiplane())
Contrary to the one in Init(), this call to isMultiplane() can be moved out of the loop without dupl […]
Ack
Patch Set #2, Line 958:
if (isMultiplane()) {
I feel like this isMultiPlane() (and the one in set_payload_size()) should be outside of the loop, s […]
Hmm. I can do that if you insist, but I don't see how I could move that condition out of the loop without duplicating the loop body entirely, as the start_, lenth_ and payload_size_ arrays are all used for single plane as well. I wrote that function so that both cases share as much code as possible.
Given that isMultiplane() is very cheap and this code is not even on the hot path, I wonder if that rework would actually improve anything.
I could add that sanity check for consistency though. WDYT?
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Mack (Gerrit)
Alexandre Courbot (Gerrit)
2 comments:
File media/capture/video/linux/v4l2_capture_delegate.cc:
Patch Set #2, Line 340:
for (int i = 0; i < num_planes_; ++i) {Ack
Apologies - your code was actually correct, for the reason I explain in my latest comment for Init(). You will probably want to revert the change you did to this block in v3.
Patch Set #2, Line 958:
unsigned int length, offset;Hmm. […]
Actually, let me withdraw that comment. From an API point of view, a device can perfectly use the multi-planar API yet only use one plane. So it turns out that your code is the correct way to handle this case as well.
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Mack (Gerrit)
I really hate nagging, and I don't want to appear obnoxiously impatient, but I'm afraid this change set will bit-rot again. Is there anything I can do to get another round of review?
Alexandre revoked his comments on version 2, so version 3 is currently obsolete. Is it necessary to repost version 2 as new one?
1 comment:
File media/capture/video/linux/v4l2_capture_delegate.cc:
Patch Set #2, Line 958:
unsigned int length, offset;Actually, let me withdraw that comment. […]
Sure, but you can either move the condition out of the loop and duplicate code or do it inline. I personally like patch set #2 much better too though, even though I believe both are correct.
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Alexandre Courbot (Gerrit)
Patch Set 3:
(1 comment)
I really hate nagging, and I don't want to appear obnoxiously impatient, but I'm afraid this change set will bit-rot again. Is there anything I can do to get another round of review?
Alexandre revoked his comments on version 2, so version 3 is currently obsolete. Is it necessary to repost version 2 as new one?
IIUC only the latest posted patch can be sent to the CQ, so you may need to reupload the version you want to see merged. Otherwise the patch LGTM but I don't have permission to +1 sadly.
Daniel Mack (Gerrit)
Re-uploaded v2 as v4 now. Please consider another look. Thanks!
Alexandre Courbot (Gerrit)
Adding more media/ owners for review.
Chrome Cunningham (Gerrit)
Hi Daniel, sorry for the slow review. The recently added media/ owners are decode/playback experts, but probably much help on this review - Miguel has the expertise in this area. There's a big open question about testing which I've pinged on below. When the experts are happy any one of us will be able to LGTM, but the test question needs follow up first.
1 comment:
Patch Set #1, Line 21:
LINUX_V4L2_MULTI_PLANE
The change set that removed that support didn't mention any of that reasoning, or did I miss anythin […]
mcasas@ friendly ping for Christian's question and Daniel's follow up. Pls give more guidance and loop in webRTC CQ infra as needed to unblock review.
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Tomasz Figa (Gerrit)
Just 1 nit and 1 general comment. Otherwise LGTM, but I don't have any reviewer rights in Chromium.
2 comments:
File media/capture/video/linux/v4l2_capture_delegate.cc:
Patch Set #4, Line 325:
bool isMultiplane() const { return V4L2_TYPE_IS_MULTIPLANAR(buf_type_); };nit: Is there a reason to mix 3 different method naming conventions? (Init, isMultiplane, num_planes)
File media/capture/video/video_capture_device_descriptor.h:
Patch Set #1, Line 21:
LINUX_V4L2_MULTI_PLANE
mcasas@ friendly ping for Christian's question and Daniel's follow up. […]
FWIW, upstream vivi has evolved into vivid and it supports MPLANE.
It is also much more configurable and IMHO we should actually consider using it for VM testing even for single planar variant, if we don't already do it.
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Patch Set 2:
(1 comment)
I've now rebased the two patches onto the current master. I'd very much like to get this reviewed.
The comment from last round still applies though, as the code supporting this camera type is not currently unit-tested. As I said, I don't know how to do that in the Chromium project. However, given that such tests are being made a requirement for this patch set, I assume there is some mock code that tests the SPLANE cameras, and that code should be easily extendible for MPLANE types as well.
Unfortunately, I don't think there is any mock code for testing the SPLANE code. Test coverage for that code is currently achieved by running tests on special trybots in the chromium.webrtc waterfall with real cameras attached.
For getting test coverage for the MPLANE code, I see three options:
1. Adding/configuring webrtc trybots that use a physical device that uses the MPLANE code path.
2. Adding/configuring webrtc trybots that use a virtual device (vivi?) that uses the MPLANE code path.
3. Mocking out V4L2 API calls to simulate the behavior of MPLANE devices.
For 1. and 2. we need to talk to phoglund@ to see if/how we can add such trybots.
phoglund@: Could you give a quick input on how doable this seems to you?
For 1. we would need to know which device(s) to get. zonque@ is there any off-the-shelf device you know of that could be bought for this purpose?
For 2. I am not familiar with vivi or vivid, but it seems tfiga@ has expertise on that.
tfiga@, phoglund@: How easy/hard would it be to set up a webrtc bot with such virtual devices?
For 3. An example for mocking out a platform-specific video capture API for unit testing can be found for the case of Windows Media Foundation based capture here: https://cs.chromium.org/chromium/src/media/capture/video/win/video_capture_device_mf_win_unittest.cc?dr&g=0. The advantage of this approach is that it does not require any special bot configuration, so it can run on the regular commit queue.
Ideally, test coverage would include all three approaches, but I think as long as we can get at least one of them, we should be okay for landing this.
Daniel Mack (Gerrit)
Unfortunately, I don't think there is any mock code for testing the SPLANE code. Test coverage for that code is currently achieved by running tests on special trybots in the chromium.webrtc waterfall with real cameras attached.
So there is no code for automated testing of SPLANE which is the only format currently supported and the most widespread too, but merging support for exotic MPLANE cameras is problematic because no test code is provided?
Wouldn't it make more sense to add testing for both types in one go?
For getting test coverage for the MPLANE code, I see three options:
1. Adding/configuring webrtc trybots that use a physical device that uses the MPLANE code path.
2. Adding/configuring webrtc trybots that use a virtual device (vivi?) that uses the MPLANE code path.
3. Mocking out V4L2 API calls to simulate the behavior of MPLANE devices.For 1. and 2. we need to talk to phoglund@ to see if/how we can add such trybots.
phoglund@: Could you give a quick input on how doable this seems to you?For 1. we would need to know which device(s) to get. zonque@ is there any off-the-shelf device you know of that could be bought for this purpose?
The only camera system I'm aware of that uses that type of MPLANE transport is Qualcomm's camss driver, and that is only used by some Snapdragon processors, and only when they are booted with the mainline kernel (and not Android).
Not sure if it's feasible to use such an embedded platform for automated tests.
So I believe it's much easier to mock the user code for this.
2 comments:
Patch Set #4, Line 325:
bool isMultiplane() const { return V4L2_TYPE_IS_MULTIPLANAR(buf_type_); };
nit: Is there a reason to mix 3 different method naming conventions? (Init, isMultiplane, num_planes […]
The code base is largely chaotic in that area, and I merely tried to keep as close to the environment as possible and keep the change set small.
A cleanup could certainly be done, but it would touch a lot more ground than this one, so it's out of scope for this contribution.
File media/capture/video/video_capture_device_descriptor.h:
Patch Set #1, Line 21:
LINUX_V4L2_MULTI_PLANE
FWIW, upstream vivi has evolved into vivid and it supports MPLANE. […]
As I said, I really don't know how the test setup in your project works, and building the tree alone takes 7 hours on my machine already. I'd really appreciate if somebody did the testing part for this. All I can say is that the implementation as it stand in this PR does work for both multi and single planarity.
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Tomasz Figa (Gerrit)
Patch Set 4:
Patch Set 2:
(1 comment)
I've now rebased the two patches onto the current master. I'd very much like to get this reviewed.
The comment from last round still applies though, as the code supporting this camera type is not currently unit-tested. As I said, I don't know how to do that in the Chromium project. However, given that such tests are being made a requirement for this patch set, I assume there is some mock code that tests the SPLANE cameras, and that code should be easily extendible for MPLANE types as well.
Unfortunately, I don't think there is any mock code for testing the SPLANE code. Test coverage for that code is currently achieved by running tests on special trybots in the chromium.webrtc waterfall with real cameras attached.
For getting test coverage for the MPLANE code, I see three options:
1. Adding/configuring webrtc trybots that use a physical device that uses the MPLANE code path.
2. Adding/configuring webrtc trybots that use a virtual device (vivi?) that uses the MPLANE code path.
3. Mocking out V4L2 API calls to simulate the behavior of MPLANE devices.
For 1. and 2. we need to talk to phoglund@ to see if/how we can add such trybots.
phoglund@: Could you give a quick input on how doable this seems to you?For 1. we would need to know which device(s) to get. zonque@ is there any off-the-shelf device you know of that could be bought for this purpose?
For 2. I am not familiar with vivi or vivid, but it seems tfiga@ has expertise on that.
tfiga@, phoglund@: How easy/hard would it be to set up a webrtc bot with such virtual devices?
For 3. An example for mocking out a platform-specific video capture API for unit testing can be found for the case of Windows Media Foundation based capture here: https://cs.chromium.org/chromium/src/media/capture/video/win/video_capture_device_mf_win_unittest.cc?dr&g=0. The advantage of this approach is that it does not require any special bot configuration, so it can run on the regular commit queue.
Ideally, test coverage would include all three approaches, but I think as long as we can get at least one of them, we should be okay for landing this.
FYI, we're interested in 3) for Chrome OS, so we can run Chromium and our tests in Chromium bots early before potential regressions sneak into Chrome OS. I've filed crbug.com/852302 to track this.
Patrik Höglund (Gerrit)
We're trying to make the WebRTC bot configs less exotic and not more, so we'd really not like to add more camera models than the C920 devices we're already using. Try out the mock approach first like Daniel is suggesting and let me know if it doesn't work out.
Christian Fremerey (Gerrit)
Patch Set 4:
(2 comments)
Unfortunately, I don't think there is any mock code for testing the SPLANE code. Test coverage for that code is currently achieved by running tests on special trybots in the chromium.webrtc waterfall with real cameras attached.
So there is no code for automated testing of SPLANE which is the only format currently supported and the most widespread too, but merging support for exotic MPLANE cameras is problematic because no test code is provided?
What is problematic about merging the MPLANE code as is is that there would be zero test coverage inside Chromium. I agree that it is bad that there is no test code for SPLANE, but I guess it has been decided at some point in the past that this was acceptable considering there is test coverage through tests involving real cameras. Since we don't have such coverage for MPLANE, and it looks like it is difficult to add, we now have no choice but to bite the bullet and do the work.
> Wouldn't it make more sense to add testing for both types in one go?
Yes, absolutely. Once we have the V4L2 calls mocked out, it should be used for testing both SPLANE and MPLANE.
For getting test coverage for the MPLANE code, I see three options:
1. Adding/configuring webrtc trybots that use a physical device that uses the MPLANE code path.
2. Adding/configuring webrtc trybots that use a virtual device (vivi?) that uses the MPLANE code path.
3. Mocking out V4L2 API calls to simulate the behavior of MPLANE devices.
So based on everyone's comments, it looks like approach 3 is the way to go.
If noone else can spend the resources to do this, I can volunteer to give it a try. But I won't be able to make this a very high priority, and likely won't be able to start before next week.
Tomasz Figa (Gerrit)
Patch Set 4:
Patch Set 4:
(2 comments)
Unfortunately, I don't think there is any mock code for testing the SPLANE code. Test coverage for that code is currently achieved by running tests on special trybots in the chromium.webrtc waterfall with real cameras attached.
So there is no code for automated testing of SPLANE which is the only format currently supported and the most widespread too, but merging support for exotic MPLANE cameras is problematic because no test code is provided?
What is problematic about merging the MPLANE code as is is that there would be zero test coverage inside Chromium. I agree that it is bad that there is no test code for SPLANE, but I guess it has been decided at some point in the past that this was acceptable considering there is test coverage through tests involving real cameras. Since we don't have such coverage for MPLANE, and it looks like it is difficult to add, we now have no choice but to bite the bullet and do the work.
> Wouldn't it make more sense to add testing for both types in one go?Yes, absolutely. Once we have the V4L2 calls mocked out, it should be used for testing both SPLANE and MPLANE.
For getting test coverage for the MPLANE code, I see three options:
1. Adding/configuring webrtc trybots that use a physical device that uses the MPLANE code path.
2. Adding/configuring webrtc trybots that use a virtual device (vivi?) that uses the MPLANE code path.
3. Mocking out V4L2 API calls to simulate the behavior of MPLANE devices.So based on everyone's comments, it looks like approach 3 is the way to go.
If noone else can spend the resources to do this, I can volunteer to give it a try. But I won't be able to make this a very high priority, and likely won't be able to start before next week.
Uhm, I just realized that I mixed up the numbering. We're interested in 2), using vivid for testing on Chrome OS builds of Chromium.
Mocking out V4L2 API calls in Chromium wouldn't work for us, since in addition to the Linux VCD that talks directly to V4L2, depending on the board configuration, we also use Chrome OS VCD that talks to a Chrome OS camera service, which then talks to V4L2.
Daniel Mack (Gerrit)
I have now rebased the patch set to Chromium 65, as that is what's bundled with Qt 5.11. While doing that, I noted a regression that happened in the rebase, so the code as it currently stands doesn't quite do the right thing. I'm working on a fix and will repost a new set once that's finished.
Christian Fremerey (Gerrit)
Patch Set 4:
Patch Set 4:
Patch Set 4:
(2 comments)
Unfortunately, I don't think there is any mock code for testing the SPLANE code. Test coverage for that code is currently achieved by running tests on special trybots in the chromium.webrtc waterfall with real cameras attached.
So there is no code for automated testing of SPLANE which is the only format currently supported and the most widespread too, but merging support for exotic MPLANE cameras is problematic because no test code is provided?
What is problematic about merging the MPLANE code as is is that there would be zero test coverage inside Chromium. I agree that it is bad that there is no test code for SPLANE, but I guess it has been decided at some point in the past that this was acceptable considering there is test coverage through tests involving real cameras. Since we don't have such coverage for MPLANE, and it looks like it is difficult to add, we now have no choice but to bite the bullet and do the work.
> Wouldn't it make more sense to add testing for both types in one go?Yes, absolutely. Once we have the V4L2 calls mocked out, it should be used for testing both SPLANE and MPLANE.
For getting test coverage for the MPLANE code, I see three options:
1. Adding/configuring webrtc trybots that use a physical device that uses the MPLANE code path.
2. Adding/configuring webrtc trybots that use a virtual device (vivi?) that uses the MPLANE code path.
3. Mocking out V4L2 API calls to simulate the behavior of MPLANE devices.So based on everyone's comments, it looks like approach 3 is the way to go.
If noone else can spend the resources to do this, I can volunteer to give it a try. But I won't be able to make this a very high priority, and likely won't be able to start before next week.Uhm, I just realized that I mixed up the numbering. We're interested in 2), using vivid for testing on Chrome OS builds of Chromium.
Mocking out V4L2 API calls in Chromium wouldn't work for us, since in addition to the Linux VCD that talks directly to V4L2, depending on the board configuration, we also use Chrome OS VCD that talks to a Chrome OS camera service, which then talks to V4L2.
Acknowledged.
I still think it makes sense to pursue approach 3), i.e. mocking out V4L2 API calls, since it allows running tests anywhere without any special configuration or kernel module needed. Also it allows very fine control of how the API calls respond directly from the test code.
As a first step, I created a CL for abstracting out the V4L2 calls, see https://chromium-review.googlesource.com/c/chromium/src/+/1120856. Next step would be to create a configurable fake implementation of that interface and use it in actual tests.
Christian Fremerey (Gerrit)
2 comments:
Patch Set #4, Line 325:
bool isMultiplane() const { return V4L2_TYPE_IS_MULTIPLANAR(buf_type_); };
The code base is largely chaotic in that area, and I merely tried to keep as close to the environmen […]
It is not as chaotic as it may seem. As per Chromium coding style guide [1], Init() and num_planes() are correct, but isMultiplane() should be IsMultiplane().
From the methods below, start() and payload_size() may still qualify as accessors, so lower-case is fine. set_payload_size() now contains quite a bit of logic, so should be changed to SetPayloadSize().
[1] https://google.github.io/styleguide/cppguide.html#Function_Names
Patch Set #4, Line 903:
capture_format_, rotation_, now, timestamp);This does not seem to make sense to me. What this would do is to send one separate video frame per plane down the video capture stack. I don't think that is what we want.
It looks like back in the day [1], VideoCaptureDeviceClient used to have a method OnIncomingCapturedYuvData() which accepted one pointer per plane and interpreted them as one frame. We would need to add something like this back here, or we would need to ask |client_| for a buffer and copy the data into the buffer from here.
[1] https://codereview.chromium.org/1026073002/patch/20001/30016
To view, visit change 742845. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Quick update regarding testing via mocking out V4L2. I created a series of CLs for making this happen. That should create the basis on top of which adding testing for the MPLANE case should be relatively easy.
See design doc at
https://docs.google.com/document/d/1ihGDZloUGdDpZ5XfmiI3AcqsSxOP9kOe5GxXOTqpHW4/edit?usp=sharing