Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
967793002:
Linux Video Capture: Add V4L2VideoCaptureDelegate{Single,Multi}Plane. (Closed)
|
Created:
5 years, 9 months ago by mcasas Modified:
5 years, 9 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux Video Capture: Add V4L2VideoCaptureDelegate{Single,Multi}Plane.
This CL adds support for V4L2 MPLANE Capture Api.
Only supported format is YUV420M triplanar.
A new method is added to VideoCaptureDeviceClient,
namely OnIncomingCapturedYuvData(...), which forces
adding MOCKing here and there, and its own
implementation.
V4L2 MMAP API works via user mmap()ing a number of
buffer allocated by V4L2 capture device. If those
buffers are not correctly munmap()ed, bad things (c)
happen. In light of this, the manual buffer lifetime
management is changed to automatic one. Construction
(mmap()ing) of those called BufferTracker is
planarity specific (i.e. there's one such ctor in
each of BufferTracker{S,M}Plane), while the dtor
is generic and the same.
ToT class diagram:
+------------------------------------+
| VideoCaptureDeviceLinux |
| +----------------------+|
| <<ref>> -->| V4L2CaptureDelegate ||
| cnt | (struct Buffer) ||
| +----------------------+|
+------------------------------------+
This CL class scheme:
+--------------------------+
| VideoCaptureDeviceLinux |
| |
| <<ref_cnt>> ---+ |
+----------------|---------+
+----------------v-----------+ v4l2_capture_delegate.{cc,h}
| +-----------------------+ |
| |V4L2CaptureDelegate | |
| | (class BufferTracker)| |
| +-----------------------+ |
+-------^------------------^-+
| |
+----|-------+ +--------|--+ v4l2_capture_delegate_multi_plane.{cc,h}
| SPlane | | MPlane |
| (BTSplane) | | (BTMPlane)|
| | +-----------+
+------------+ v4l2_capture_delegate_single_plane.{cc,h}
- VCDevice works on the premise that its calls into
VCDevice::Client::OnIncomingWhatever() are synchronous.
That assumption is respected here.
- A bit of cleanup is done in OnIncomingCaptureData(),
in what regards rotation/crop/odd sizes. A unit test
is subsequently added.
- VideoCaptureDeviceFactory labels the devices as
Single or Multi Planar. That labeling capture_api_type()
needs to ripple through a bunch of files, causing some
otherwise uninteresting changes in the patchsets.
BUG=441836
TEST= Compile and insmod vivid.ko into a kernel,
with options for supporting MPLANE api (multiplanar=2)
then capture using patched Chromium. Current vivid
does _not_ support any common Mplane format, needs
a patch:
https://github.com/miguelao/linux/tree/adding_yu12_yv12_nv12_nv21___mplane_formats___with_mods_for_kernel_3_13
that needs to be compiled against ubuntu sources 3.13 etc.
For even better coverage, use a normal WebCam and
navigate to http://goo.gl/fUcIiP, then open both
the MPlane camera mentioned in the previous paragraph
and the "normal" webcam (this is,partially, how I
try it).
Committed: https://crrev.com/d3d37d0f3493dd28cd687fe9285b0aac81a61dae
Cr-Commit-Position: refs/heads/master@{#321612}
Patch Set 1 : #
Total comments: 55
Patch Set 2 : Rebased (VideoCaptureControllerClient splitted from VCController) #Patch Set 3 : emircan@ and posciak@ comments #
Total comments: 10
Patch Set 4 : magjed@ comments #
Total comments: 48
Patch Set 5 : posciak@ comments #
Total comments: 27
Patch Set 6 : Extracted V4L2CaptureDelegate{Single,Multi}Plane into separated files, Multi not compiled for OpenB… #Patch Set 7 : posciak@ comments on PS5. Minor rebase of video_capture_controller_unittest.cc and BUILD.gn #
Total comments: 28
Patch Set 8 : posciak@ nits perkj@,magjed@ and dalecurtis@ comments. Rebase. #
Total comments: 4
Patch Set 9 : DCHECK correction. Rebase. #
Total comments: 4
Patch Set 10 : s/V4L2VideoCaptureDelegate/V4L2CaptureDelegate/; using v4l2_format for strides. #
Total comments: 4
Patch Set 11 : magjed@ comments #Messages
Total messages: 61 (31 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
mcasas@chromium.org changed reviewers: + emircan@chromium.org
emircan@ PTAL
I'll need more time to understand general workflow of this. https://codereview.chromium.org/967793002/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/967793002/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:434: rotation_mode = libyuv::kRotate270; Consider a switch statement. https://codereview.chromium.org/967793002/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:576: scoped_refptr<Buffer> buffer = ReserveOutputBuffer(VideoFrame::I420, Consider using a scoped_ptr and passing ownership. However, it would require function signature change on VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread, and may be handled in another CL. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:205: std::list<uint32_t> V4L2VideoCaptureDelegate::GetListOfUsableFourCss( /s/FourCss/FourCcs/ https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:287: } You can end the iteration when (best == desired_v4l2_formats.begin()). https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.h (right): https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:39: static std::list<uint32_t> GetListOfUsableFourCss(bool prefer_mjpeg); /s/FourCss/FourCcs/
mcasas@chromium.org changed reviewers: + posciak@chromium.org
posciak@ PTAL (emircan@ I'll take on your comments , since they are few, together with posciak@s in the next patchset).
posciak@chromium.org changed reviewers: + perkj@chromium.org
I only had enough steam to finish Delegates for now. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:95: BufferTrackerSPlane(int fd, const v4l2_buffer& buffer); We need Initialize() methods as I mention elsewhere in order to return failures. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:172: if (pixel_format != PIXEL_FORMAT_I420) This may bitrot. Please instead look up pixel_format in the supported formats array above. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:213: multiplane_formats.insert( if (prefer_mjpeg) multiplane_formats.push_front(MJPEG); else multiplane_formats.push_back(MJPEG); https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:215: V4L2_PIX_FMT_MJPEG); This is different from existing code, which always has JPEG last, MJPEG always comes before JPEG. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:219: V4L2VideoCaptureDelegate::BufferTracker::BufferTracker( This is not needed? https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:286: best = std::find(desired_v4l2_formats.begin(), best, fmtdesc.pixelformat); Maybe I'm missing something, but I think this will for example fail if we have MJPEG at the front of desired_v4l2_formats and ENUM_FMT returns in order: MJPEG and XYZ. Then the first iteration will get best = desired_v4l2_formats.begin(), because MJPEG is at beginning. Then second iteration will call an equivalent of best = std::find(desired_v4l2_formats.begin(), desired_v4l2_formats.begin(), fmtdesc.pixelformat), which is an empty list and will always fail. So best = end() at the end and we will always fail, even though we should have matched MJPEG? https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:344: V4l2FourCcToChromiumPixelFormat(video_fmt.fmt.pix.pixelformat); This needs to be checked for success, we don't want to proceed with a bad format. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:416: } As described in h, this all could go to CreateBufferTracker please. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:462: FinishFillingV4L2Buffer(&buffer); This would just be FillV4L2Buffer(). https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:559: v4l2_plane_.reset(new v4l2_plane[num_planes_]); We still don't want to call new foo[0] if GetNumPlanesForFourCc fails, as per standard: "The effect of dereferencing a pointer returned as a request for zero size is undefined." https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:597: return; This way you'd still use the plane as normal if this failed. You need an Initialize() method. Also, you can't leave plane->start = MAP_FAILED, MAP_FAILED is not NULL, so when unmapping, we'll succeed plane->start == NULL test and we'll end up calling munmap(MAP_FAILED, 0); https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... File media/video/capture/linux/v4l2_video_capture_delegate.h (right): https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:9: #include <sys/videoio.h> As mentioned in previous PS, this is does not appear to be full V4L2 and as far as I can tell from what I can see of this file in Google search it does not support MPLANE and other new API elements. That said, I don't know if we support OpenBSD in general. You probably need to figure out what the status of that is. We may have to make your MPLANE changes only compile on non-BSD, but I don't know how we would verify that. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:34: // Returns the Cr pixel format for |v4l2_fourcc| or PIXEL_FORMAT_UNKNOWN. s/Cr/Chromium/ https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:52: // and munmap()ed on destruction. Destruction is syntactically equalfor s/equalfor/equal for/ https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:56: class BufferTracker : public base::RefCounted<BufferTracker> { You should need to only declare this class here in .h and move impl to cc. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:65: ScopedVector<Plane>& planes() { return planes_; } In what case would you like to use a non-const accessor? It would actually be a bug, because clients cannot change the values, otherwise things would explode. Also, I'd recommend having accessors like this: uint8_t PlaneStart(size_t plane) const { return static_cast<uint8_t*>(planes_[plane].start); } size_t PlaneLength(size_t plane) const { return planes_[plane].length; } You'd then instead of: static_cast<uint8_t>(buffer_tracker->planes()[0]->start) have: buffer_tracker->PlaneStart(0) etc. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:73: ScopedVector<Plane> planes_; Plane does not have a specialized destructor and does not really have to be created via operator new. You can just have std::vector<Plane> here. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:82: // Creates the necessary, planarity-specific, internal tracking schemes. Please document arguments. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:93: // Finish filling |buffer| struct with planarity-dependent data. "Finish" doesn't really explain what this does and the function name already says all this... But you could have: V4L2VideoCaptureDelegate::FillV4L2Buffer(v4l2_buffer* buffer, int i) { memset(&buffer, 0, sizeof(*buffer)); buffer->memory = V4L2_MEMORY_MMAP; buffer->index = i; FinishFillingV4L2Buffer(buffer); } And have: V4L2VideoCaptureDelegateSplane::FinishFillingV4L2Buffer(v4l2_buffer* buffer) { buffer->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; } V4L2VideoCaptureDelegateMplane::FinishFillingV4L2Buffer(v4l2_buffer* buffer) { buffer->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; buffer->planes = ... buffer->length = ... } Then Delegate would only have to call FillV4L2Buffer() and not worry about anything else. Also, please move QUERYBUF to CreateBuffer, it's much simpler this way: // note capture_type_ is not needed scoped_refptr<BufferTracker> CreateBufferTracker> CreateBufferTracker(int fd, int index) { scoped_refptr<BufferTracker> buf_tracker = new BufferTracker...(); v4l2_buffer buffer; FillV4L2Buffer(&buffer, i); if (!buf_tracker->Initialize(fd, &buffer)) return nullptr; return buf_tracker; } BufferTrackerSPlane::Initialize(int fd, v4l2_buffer* buffer) { querybuf()... if (fail) return false; mmap()... if (fail) return false; return true; } For mplane, Initialize similar but also does what FinishFilling...() would. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:96: // Sends the captured |buffer| to the |client|, synchronously. s/|client|/client_/ Also, please just take scoped_ptr to BufferTracker instead of v4l2_buffer. It's the only thing you need in Send anyway, and you won't need the buffer_tracker_pool() accessor.
perkj@chromium.org changed reviewers: + magjed@chromium.org
Adding magjed. I will be ooo for a week. Sorry, I have not had time to look at this.
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
guys PTAL. I had to do a rebase, but luckily not big changes happened. https://codereview.chromium.org/967793002/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/967793002/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:434: rotation_mode = libyuv::kRotate270; On 2015/03/04 02:47:47, emircan wrote: > Consider a switch statement. Acknowledged. https://codereview.chromium.org/967793002/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:576: scoped_refptr<Buffer> buffer = ReserveOutputBuffer(VideoFrame::I420, On 2015/03/04 02:47:47, emircan wrote: > Consider using a scoped_ptr and passing ownership. However, it would require > function signature change on > VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread, and may be > handled in another CL. Good idea. Please add a bug to investigate this. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:95: BufferTrackerSPlane(int fd, const v4l2_buffer& buffer); On 2015/03/06 10:43:54, Pawel Osciak wrote: > We need Initialize() methods as I mention elsewhere in order to return failures. Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:172: if (pixel_format != PIXEL_FORMAT_I420) On 2015/03/06 10:43:54, Pawel Osciak wrote: > This may bitrot. Please instead look up pixel_format in the supported formats > array above. I think this early-bail-out is not really needed anymore. Best to leave this factory method to create a V4L2 delegate of the appropriate API, and, if the pixel format is not supported, fail in AllocateAndStart() which is more orthodox, i.e. follows other platform implementations modus operandi. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:205: std::list<uint32_t> V4L2VideoCaptureDelegate::GetListOfUsableFourCss( On 2015/03/04 02:47:47, emircan wrote: > /s/FourCss/FourCcs/ Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:213: multiplane_formats.insert( On 2015/03/06 10:43:54, Pawel Osciak wrote: > if (prefer_mjpeg) > multiplane_formats.push_front(MJPEG); > else > multiplane_formats.push_back(MJPEG); Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:215: V4L2_PIX_FMT_MJPEG); On 2015/03/06 10:43:55, Pawel Osciak wrote: > This is different from existing code, which always has JPEG last, MJPEG always > comes before JPEG. Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:219: V4L2VideoCaptureDelegate::BufferTracker::BufferTracker( On 2015/03/06 10:43:55, Pawel Osciak wrote: > This is not needed? Needed due to: [chromium-style] Complex class/struct needs an explicit out-of-line constructor. But now, with an Init(...) method, it's not needed. Removed. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:286: best = std::find(desired_v4l2_formats.begin(), best, fmtdesc.pixelformat); On 2015/03/06 10:43:55, Pawel Osciak wrote: > Maybe I'm missing something, but I think this will for example fail if we have > MJPEG at the front of desired_v4l2_formats and ENUM_FMT returns in order: MJPEG > and XYZ. > > Then the first iteration will get best = desired_v4l2_formats.begin(), because > MJPEG is at beginning. Then second iteration will call an equivalent of best = > std::find(desired_v4l2_formats.begin(), desired_v4l2_formats.begin(), > fmtdesc.pixelformat), which is an empty list and will always fail. So best = > end() at the end and we will always fail, even though we should have matched > MJPEG? According to the fact that it works and the list of possible equivalent implementations in [1], Iterator a = foo.begin(); Iterator b = foo.begin(); std::find(a, b, bla) is |b|. So you could argue that the code as is, could do several loop iterations after finding the best format. emircan@ has a similar comment below. [1] http://en.cppreference.com/w/cpp/algorithm/find https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:287: } On 2015/03/04 02:47:47, emircan wrote: > You can end the iteration when (best == desired_v4l2_formats.begin()). That would be an overoptimization, wouldn't it? Meaning, let's say desired_v4l2_formats is { V4L2_PIX_FMT_MJPEG, V4L2_PIX_FMT_UYVY }; and the webcam only supports, say, UYVY. We would iterate two times and select UYVY at the end. No early-exiting the for loop, since the early formats are not the optimal. (Conversely, if the webcam would support MJPEG, then early exit would be beneficial). Since this loop is only run once and during capture startup phase, I'd say we leave it as is. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:344: V4l2FourCcToChromiumPixelFormat(video_fmt.fmt.pix.pixelformat); On 2015/03/06 10:43:55, Pawel Osciak wrote: > This needs to be checked for success, we don't want to proceed with a bad > format. Done, but moved a bit farther above. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:416: } On 2015/03/06 10:43:54, Pawel Osciak wrote: > As described in h, this all could go to CreateBufferTracker please. Acknowledged. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:462: FinishFillingV4L2Buffer(&buffer); On 2015/03/06 10:43:54, Pawel Osciak wrote: > This would just be FillV4L2Buffer(). Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:559: v4l2_plane_.reset(new v4l2_plane[num_planes_]); On 2015/03/06 10:43:55, Pawel Osciak wrote: > We still don't want to call new foo[0] if GetNumPlanesForFourCc fails, as per > standard: > > "The effect of dereferencing a pointer returned as a request for zero size is > undefined." Done. I return a bool (false) if this goes south and then on AllocateAndStart() is caught and treated. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:597: return; On 2015/03/06 10:43:55, Pawel Osciak wrote: > This way you'd still use the plane as normal if this failed. You need an > Initialize() method. > > Also, you can't leave plane->start = MAP_FAILED, MAP_FAILED is not NULL, so when > unmapping, we'll succeed plane->start == NULL test and we'll end up calling > munmap(MAP_FAILED, 0); Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.h (right): https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:9: #include <sys/videoio.h> On 2015/03/06 10:43:55, Pawel Osciak wrote: > As mentioned in previous PS, this is does not appear to be full V4L2 and as far > as I can tell from what I can see of this file in Google search it does not > support MPLANE and other new API elements. > > That said, I don't know if we support OpenBSD in general. You probably need to > figure out what the status of that is. We may have to make your MPLANE changes > only compile on non-BSD, but I don't know how we would verify that. There are no OpenBSD bots. My guess is that Chromium is patched before being packaged for those distros. I could preprocessor-filter out MPLANE parts. It'd be dirty, so I'd rather do it if there's real need for it. Any suggestions? https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:34: // Returns the Cr pixel format for |v4l2_fourcc| or PIXEL_FORMAT_UNKNOWN. On 2015/03/06 10:43:55, Pawel Osciak wrote: > s/Cr/Chromium/ Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:39: static std::list<uint32_t> GetListOfUsableFourCss(bool prefer_mjpeg); On 2015/03/04 02:47:47, emircan wrote: > /s/FourCss/FourCcs/ Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:52: // and munmap()ed on destruction. Destruction is syntactically equalfor On 2015/03/06 10:43:55, Pawel Osciak wrote: > s/equalfor/equal for/ Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:56: class BufferTracker : public base::RefCounted<BufferTracker> { On 2015/03/06 10:43:55, Pawel Osciak wrote: > You should need to only declare this class here in .h and move impl to cc. Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:65: ScopedVector<Plane>& planes() { return planes_; } On 2015/03/06 10:43:55, Pawel Osciak wrote: > In what case would you like to use a non-const accessor? It would actually be a > bug, because clients cannot change the values, otherwise things would explode. > It's needed in the ctor of derived classes, f.i. for doing things like: planes().push_back(plane.Pass()); since |planes_| cannot be accessed directly from derived classes. > Also, I'd recommend having accessors like this: > uint8_t PlaneStart(size_t plane) const { return > static_cast<uint8_t*>(planes_[plane].start); } > size_t PlaneLength(size_t plane) const { return planes_[plane].length; } > Good suggestion, in that case I don't need the const accessor. > You'd then instead of: > static_cast<uint8_t>(buffer_tracker->planes()[0]->start) > have: > buffer_tracker->PlaneStart(0) > etc. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:73: ScopedVector<Plane> planes_; On 2015/03/06 10:43:55, Pawel Osciak wrote: > Plane does not have a specialized destructor and does not really have to be > created via operator new. You can just have std::vector<Plane> here. Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:82: // Creates the necessary, planarity-specific, internal tracking schemes. On 2015/03/06 10:43:55, Pawel Osciak wrote: > Please document arguments. Done. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:93: // Finish filling |buffer| struct with planarity-dependent data. On 2015/03/06 10:43:55, Pawel Osciak wrote: > "Finish" doesn't really explain what this does and the function name already > says all this... > > But you could have: > > V4L2VideoCaptureDelegate::FillV4L2Buffer(v4l2_buffer* buffer, int i) { > memset(&buffer, 0, sizeof(*buffer)); > buffer->memory = V4L2_MEMORY_MMAP; > buffer->index = i; > FinishFillingV4L2Buffer(buffer); > } > > And have: > > V4L2VideoCaptureDelegateSplane::FinishFillingV4L2Buffer(v4l2_buffer* buffer) { > buffer->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > } > > V4L2VideoCaptureDelegateMplane::FinishFillingV4L2Buffer(v4l2_buffer* buffer) { > buffer->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > buffer->planes = ... > buffer->length = ... > } > > Then Delegate would only have to call FillV4L2Buffer() and not worry about > anything else. > Done. > > Also, please move QUERYBUF to CreateBuffer, it's much simpler this way: > > // note capture_type_ is not needed > scoped_refptr<BufferTracker> CreateBufferTracker> CreateBufferTracker(int fd, > int index) { > scoped_refptr<BufferTracker> buf_tracker = new BufferTracker...(); > v4l2_buffer buffer; > FillV4L2Buffer(&buffer, i); > if (!buf_tracker->Initialize(fd, &buffer)) > return nullptr; > > return buf_tracker; > } > > BufferTrackerSPlane::Initialize(int fd, v4l2_buffer* buffer) { > querybuf()... > if (fail) > return false; > mmap()... > if (fail) > return false; > > return true; > } > > For mplane, Initialize similar but also does what FinishFilling...() would. I just wanted to move as much common code as possible to the boilerplate in V4L2CaptureDelegate. I propose something similar, hopefully covering your suggestions: void AllocateAndStart(...) { ... // VIDIOC_REQBUFS for_each(buffer) if (!AllocateVideoBuffer(index)) SetErrorState + return } bool AllocateVideoBuffer(...) { // QUERYBUF auto buffer_tracker = CreateBufferTracker() // Pure factory. if (!buffer_tracker->Init(...)) error_and_return // QBUF } BufferTrackerSPlane/BufferTrackerMPlane::CreateBufferTracker() { // return a BufferTracker{S,M}Plane } BufferTrackerSPlane/BufferTrackerMPlane::Init( int fd, const v4l2_buffer& buffer) { // do the mmap() here. } Which is similar to what you propose, with a few twists to acommmodate the planarity/method derivation etc. https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:96: // Sends the captured |buffer| to the |client|, synchronously. On 2015/03/06 10:43:55, Pawel Osciak wrote: > s/|client|/client_/ > > Also, please just take scoped_ptr to BufferTracker instead of v4l2_buffer. It's > the only thing you need in Send anyway, and you won't need the > buffer_tracker_pool() accessor. Done.
https://codereview.chromium.org/967793002/diff/180001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/967793002/diff/180001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:672: unsigned char data[kScratchpadSizeInBytes]; You need to initialize this memory to avoid DrMemory errors. https://codereview.chromium.org/967793002/diff/180001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/967793002/diff/180001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:261: libyuv::CopyPlane(y_data, y_stride, dst_y, y_stride, How about libyuv::I420Copy instead? https://codereview.chromium.org/967793002/diff/180001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:272: base::Closure()); Don't you need to return the buffer in this closure? https://codereview.chromium.org/967793002/diff/180001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://codereview.chromium.org/967793002/diff/180001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:629: bool V4L2CaptureDelegateMultiPlane::BufferTrackerMPlane::Init( This function is very similar to V4L2CaptureDelegateSinglePlane::BufferTrackerSPlane::Init. Can you extract the common parts? https://codereview.chromium.org/967793002/diff/180001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/967793002/diff/180001/media/video/capture/vid... media/video/capture/video_capture_device.h:209: virtual void OnIncomingCapturedYuvData( I need a similar function to enable planar support for Mac, e.g. NV12. How about generalizing to: OnIncomingCapturedPlanarData( const uint8_t* data[media::VideoFrame::kMaxPlanes], int32 strides[media::VideoFrame::kMaxPlanes], ... ? In any case, the input data should be const, and I think it's more consistent to the rest of the code to send in strides instead of lengths. Also, the function should be pure virtual.
Patchset #4 (id:200001) has been deleted
posciak@ PTAL magjed@ PTAL if you wish. https://codereview.chromium.org/967793002/diff/180001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/967793002/diff/180001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:672: unsigned char data[kScratchpadSizeInBytes]; On 2015/03/10 16:32:47, magjed wrote: > You need to initialize this memory to avoid DrMemory errors. Done. https://codereview.chromium.org/967793002/diff/180001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/967793002/diff/180001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:261: libyuv::CopyPlane(y_data, y_stride, dst_y, y_stride, On 2015/03/10 16:32:47, magjed wrote: > How about libyuv::I420Copy instead? Done. It amounts to a similar thing at the end. https://codereview.chromium.org/967793002/diff/180001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:272: base::Closure()); On 2015/03/10 16:32:47, magjed wrote: > Don't you need to return the buffer in this closure? Which buffer? (hehe) Ok, seriously if you are talking about the VideoCaptureBufferPool::Buffer, then that is dealt with in DoIncomingCapturedVideoFrameOnIOThread() by ReserveForProducers(). It basically "takes care of itself". This Closure is for cleaning up resource associated to the VideoFrame and, in particular, to wait for any SyncPoints, which we don't have. https://codereview.chromium.org/967793002/diff/180001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://codereview.chromium.org/967793002/diff/180001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:629: bool V4L2CaptureDelegateMultiPlane::BufferTrackerMPlane::Init( On 2015/03/10 16:32:48, magjed wrote: > This function is very similar to > V4L2CaptureDelegateSinglePlane::BufferTrackerSPlane::Init. Can you extract the > common parts? It is indeed similar. IMHO the differences are large enough to merit a different implementation, in particular that here we need to use buffer.m.planes[p].length and buffer.m.planes[p].m.mem_offset whereas in a SPlane implementation we use buffer.length and buffer.m.offset. Also, we have different amount of planes of course, but that could be passed as parameter or other similar idiom. https://codereview.chromium.org/967793002/diff/180001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/967793002/diff/180001/media/video/capture/vid... media/video/capture/video_capture_device.h:209: virtual void OnIncomingCapturedYuvData( On 2015/03/10 16:32:48, magjed wrote: > I need a similar function to enable planar support for Mac, e.g. NV12. How about > generalizing to: > OnIncomingCapturedPlanarData( > const uint8_t* data[media::VideoFrame::kMaxPlanes], > int32 strides[media::VideoFrame::kMaxPlanes], > ... > ? In the current code we only need I420 triplanar, i.e. doing a copy-to-buffer-allocated, whereas biplanar would need a deinterlace step, most likely via the same ConverToI420() method in use now. IOW perhaps the refactoring would mean merging both current implementations OnIncomingCapturedData() and the proposed OnIncomingCapturedYuvData() into a single one. I dig that but I think is beyond the scope of this CL. I would be happy to work on it right after, though. Let's make a bug or similar. > In any case, the input data should be const, Done. > and I think it's more consistent to > the rest of the code to send in strides > instead of lengths. Also, the function > should be pure virtual. OnIncomingCapturedData() has no stride information. Perhaps for a consolidation-followup CL?
Patchset #4 (id:200002) has been deleted
https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:172: if (pixel_format != PIXEL_FORMAT_I420) On 2015/03/09 21:23:56, mcasas wrote: > On 2015/03/06 10:43:54, Pawel Osciak wrote: > > This may bitrot. Please instead look up pixel_format in the supported formats > > array above. > > I think this early-bail-out is not really needed > anymore. Best to leave this factory method to > create a V4L2 delegate of the appropriate API, > and, if the pixel format is not supported, fail > in AllocateAndStart() which is more orthodox, i.e. > follows other platform implementations modus > operandi. Acknowledged. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:286: best = std::find(desired_v4l2_formats.begin(), best, fmtdesc.pixelformat); On 2015/03/09 21:23:56, mcasas wrote: > On 2015/03/06 10:43:55, Pawel Osciak wrote: > > Maybe I'm missing something, but I think this will for example fail if we have > > MJPEG at the front of desired_v4l2_formats and ENUM_FMT returns in order: > MJPEG > > and XYZ. > > > > Then the first iteration will get best = desired_v4l2_formats.begin(), because > > MJPEG is at beginning. Then second iteration will call an equivalent of best = > > std::find(desired_v4l2_formats.begin(), desired_v4l2_formats.begin(), > > fmtdesc.pixelformat), which is an empty list and will always fail. So best = > > end() at the end and we will always fail, even though we should have matched > > MJPEG? > > According to the fact that it works and the list > of possible equivalent implementations in [1], > > Iterator a = foo.begin(); > Iterator b = foo.begin(); > std::find(a, b, bla) > > is |b|. > > So you could argue that the code as is, could do > several loop iterations after finding the best format. > emircan@ has a similar comment below. > > > > [1] http://en.cppreference.com/w/cpp/algorithm/find Acknowledged and true. Sorry. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... File media/video/capture/linux/v4l2_video_capture_delegate.h (right): https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:9: #include <sys/videoio.h> On 2015/03/09 21:23:57, mcasas wrote: > On 2015/03/06 10:43:55, Pawel Osciak wrote: > > As mentioned in previous PS, this is does not appear to be full V4L2 and as > far > > as I can tell from what I can see of this file in Google search it does not > > support MPLANE and other new API elements. > > > > That said, I don't know if we support OpenBSD in general. You probably need to > > figure out what the status of that is. We may have to make your MPLANE changes > > only compile on non-BSD, but I don't know how we would verify that. > > There are no OpenBSD bots. > > My guess is that Chromium is patched before being packaged > for those distros. > > I could preprocessor-filter out MPLANE parts. It'd > be dirty, so I'd rather do it if there's real need > for it. Any suggestions? This CL may breaking these platforms (may not even compile), so there is a real need. As for preprocessor-filtering, we are already using two separate classes, hopefully all specifics can go in there. We'd have them in separate files and not compile the mplane class' file if not supported. The only ifdefs in the code would hopefully be in a factory method in the base class. That said, I don't know if all splane features are supported in videoio.h. I don't know if it still works as is right now. And I'm not sure how you'd compile-test this if you don't have an OpenBSD environment. Perhaps a good idea would be to ask the original author who added this and/or on chromium-dev. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:65: ScopedVector<Plane>& planes() { return planes_; } On 2015/03/09 21:23:57, mcasas wrote: > On 2015/03/06 10:43:55, Pawel Osciak wrote: > > In what case would you like to use a non-const accessor? It would actually be > a > > bug, because clients cannot change the values, otherwise things would explode. > > > > It's needed in the ctor of derived classes, f.i. for > doing things like: > > planes().push_back(plane.Pass()); > > since |planes_| cannot be accessed directly > from derived classes. > Right, the point is we don't want the users of this class to be able to modify planes. So we need to make the accessor protected, if it can't be public const. But we could just simply make planes_ protected instead and remove the accessor altogether. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/967793002/diff/220001/media/media.gyp#... media/media.gyp:579: 'video/capture/linux/video_capture_device_chromeos.cc', Not in order. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:86: struct Plane { This doesn't need to be public. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:90: virtual bool Init(int fd, const v4l2_buffer& buffer) = 0; Documentation please. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:97: std::vector<Plane>& planes() { return planes_; } As mentioned before, this shouldn't be public non-const. We can either do planes_ protected or the accessor protected, but the latter doesn't make much sense. We have GetPlaneStart and GetPlaneLength for public. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:125: ~BufferTrackerSPlane() override {}; s/;// https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:128: ~V4L2CaptureDelegateSinglePlane() override {}; s/;// https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:160: ~BufferTrackerMPlane() override {}; s/;// https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:163: ~V4L2CaptureDelegateMultiPlane() override {}; s/;// https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:175: uint32_t fourcc_; We don't seem to be using it anywhere but in FillV4L2Format(), so this member is not needed? https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:176: size_t num_planes_; Better use v4l2_plane_.size() and remove this, also guards us from somehow getting a different number of planes in v4l2_plane_[] than num_planes_. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:209: {V4L2_PIX_FMT_YUV420M, PIXEL_FORMAT_I420}, Could we please have one array instead of three (kFourCcAndChromiumPixelFormats, kSinglePlaneSupportedFormats, kMultiPlaneSupportedFormats)? struct { uint32_t fourcc; VideoPixelFormat pixel_format; size_t num_v4l2_planes; } https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:247: if (plane.start == NULL) s/NULL/nullptr/ https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:364: (power_line_frequency_ == V4L2_CID_POWER_LINE_FREQUENCY_60HZ)) { What if it's V4L2_CID_POWER_LINE_FREQUENCY_AUTO? We still want to set it I think? https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:368: HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_S_CTRL, &control)); Better DVLOG on failure at least, even if not fatal. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:436: bool V4L2VideoCaptureDelegate::AllocateVideoBuffer(int index) { This is not allocating, but mapping and queuing only. Allocation happens on REQBUFS. Please rename the method to MapAndQueueBuffer(i). https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:437: v4l2_buffer buffer = {}; s/ = {};/;/ Fill...() already memsets it. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:496: v4l2_buffer buffer = {}; You could replace lines 496-500 with FillV4L2Buffer(...) https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:552: reinterpret_cast<BufferTrackerSPlane*>(buffer.get()); Downcasting shouldn't be needed. GetPlaneStart/Length are virtual. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:570: DLOG(ERROR) << "Error mmap()ing a V4L2 buffer into userspace"; plane.start = nullptr; https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:607: buffer->length = num_planes_; v4l2_plane_.size() https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:616: reinterpret_cast<BufferTrackerMPlane*>(buffer.get()); Please remove downcast. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:617: client()->OnIncomingCapturedYuvData(buffer_tracker->GetPlaneStart(0), S_FMT may adjust values for plane sizes, resolution and strides. We need to at least verify that they will fit within what the client expects in OnIncomingCapturedYuvData and fail otherwise. I'm not requiring handling it for now, just checking and failing if they are not what we expect. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... File media/video/capture/linux/video_capture_device_linux.h (right): https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/video_capture_device_linux.h:30: static int TranslatePowerLineFrequencyToV4L2(int frequency); This can be private I think?
Patchset #5 (id:240001) has been deleted
Patchset #5 (id:260001) has been deleted
Patchset #5 (id:280001) has been deleted
Patchset #6 (id:320001) has been deleted
Patchset #6 (id:340001) has been deleted
posciak@ PTAL.
Note please the 2 Patchset 5 and 6. PS5 represents all
your comments addressed, PS6 is the result of extracting
V4L2CaptureDelegate{Single,Multi}Plane into files, for
the purpose of avoiding their compilation in OpenBSD. I
hope this eases the review.
https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin...
File media/video/capture/linux/v4l2_video_capture_delegate.h (right):
https://codereview.chromium.org/967793002/diff/100001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.h:65: ScopedVector<Plane>&
planes() { return planes_; }
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> On 2015/03/09 21:23:57, mcasas wrote:
> > On 2015/03/06 10:43:55, Pawel Osciak wrote:
> > > In what case would you like to use a non-const accessor? It would actually
> be
> > a
> > > bug, because clients cannot change the values, otherwise things would
> explode.
> > >
> >
> > It's needed in the ctor of derived classes, f.i. for
> > doing things like:
> >
> > planes().push_back(plane.Pass());
> >
> > since |planes_| cannot be accessed directly
> > from derived classes.
> >
>
> Right, the point is we don't want the users of this class to be able to modify
> planes. So we need to make the accessor protected, if it can't be public
const.
> But we could just simply make planes_ protected instead and remove the
accessor
> altogether.
I propose adding a protected AddMmapedPlane() method,
that will offer the only non-const operation that should
be available to children classes, i.e. adding a Plane.
In this way, Struct Plane and |planes_| are private.
More details in last patchset's comments.
https://codereview.chromium.org/967793002/diff/220001/media/media.gyp
File media/media.gyp (right):
https://codereview.chromium.org/967793002/diff/220001/media/media.gyp#newcode579
media/media.gyp:579: 'video/capture/linux/video_capture_device_chromeos.cc',
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> Not in order.
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
File media/video/capture/linux/v4l2_video_capture_delegate.cc (right):
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:86: struct Plane {
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> This doesn't need to be public.
Done, see below.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:90: virtual bool
Init(int fd, const v4l2_buffer& buffer) = 0;
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> Documentation please.
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:97: std::vector<Plane>&
planes() { return planes_; }
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> As mentioned before, this shouldn't be public non-const. We can either do
> planes_ protected or the accessor protected, but the latter doesn't make much
> sense.
> We have GetPlaneStart and GetPlaneLength for public.
(Cont'd from elsewhere)
Since the only non-const operation we want to publish
is add-a-plane-to-this-buffer-tracker, I hid the non-
const planes() accesor and wrote instead
AddMmapedPlane(start, length)
that can also be made protected.
I somehow sacrifice the nexus between start and length
during Init() in favour of not showing off the
internals of Plane tracking. I think is a good
tradeoff all in all since Init() is implemented
in derived classes and, by hiding behind this
method, both Struct Plane and |planes_| can be
made private.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:125:
~BufferTrackerSPlane() override {};
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> s/;//
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:128:
~V4L2CaptureDelegateSinglePlane() override {};
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> s/;//
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:160:
~BufferTrackerMPlane() override {};
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> s/;//
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:163:
~V4L2CaptureDelegateMultiPlane() override {};
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> s/;//
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:175: uint32_t fourcc_;
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> We don't seem to be using it anywhere but in FillV4L2Format(), so this member
is
> not needed?
Well spotted, removed.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:176: size_t
num_planes_;
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> Better use v4l2_plane_.size() and remove this, also guards us from somehow
> getting a different number of planes in v4l2_plane_[] than num_planes_.
I'm answering this in l.607 comments.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:209:
{V4L2_PIX_FMT_YUV420M, PIXEL_FORMAT_I420},
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> Could we please have one array instead of three
(kFourCcAndChromiumPixelFormats,
> kSinglePlaneSupportedFormats, kMultiPlaneSupportedFormats)?
>
> struct {
> uint32_t fourcc;
> VideoPixelFormat pixel_format;
> size_t num_v4l2_planes;
> }
I think it mixes things that pertain to different
implementations, i.e. there should be no such thing
as "number of planes" in single plane API formats.
That said, Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:247: if (plane.start ==
NULL)
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> s/NULL/nullptr/
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:364:
(power_line_frequency_ == V4L2_CID_POWER_LINE_FREQUENCY_60HZ)) {
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> What if it's V4L2_CID_POWER_LINE_FREQUENCY_AUTO? We still want to set it I
> think?
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:368:
HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_S_CTRL, &control));
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> Better DVLOG on failure at least, even if not fatal.
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:436: bool
V4L2VideoCaptureDelegate::AllocateVideoBuffer(int index) {
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> This is not allocating, but mapping and queuing only. Allocation happens on
> REQBUFS. Please rename the method to MapAndQueueBuffer(i).
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:437: v4l2_buffer buffer
= {};
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> s/ = {};/;/
> Fill...() already memsets it.
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:496: v4l2_buffer buffer
= {};
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> You could replace lines 496-500 with FillV4L2Buffer(...)
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:552:
reinterpret_cast<BufferTrackerSPlane*>(buffer.get());
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> Downcasting shouldn't be needed. GetPlaneStart/Length are virtual.
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:570: DLOG(ERROR) <<
"Error mmap()ing a V4L2 buffer into userspace";
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> plane.start = nullptr;
Actually unneeded given the change in
BufferTracker API, since we only "construct"
the Plane in AddMmapedPlane() if
start != MAP_FAILED.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:607: buffer->length =
num_planes_;
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> v4l2_plane_.size()
Actually it's not that simple. We need to have
an array in order to use it as a pointer
in the very next line (l.688) and that
prevents having length information
into e.g. a ScopedVector. IOW we still need
a member |num_planes_|. Renamed for more
clarity.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:616:
reinterpret_cast<BufferTrackerMPlane*>(buffer.get());
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> Please remove downcast.
Done.
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.cc:617:
client()->OnIncomingCapturedYuvData(buffer_tracker->GetPlaneStart(0),
On 2015/03/13 09:52:52, Pawel Osciak wrote:
> S_FMT may adjust values for plane sizes, resolution and strides. We need to at
> least verify that they will fit within what the client expects in
> OnIncomingCapturedYuvData and fail otherwise. I'm not requiring handling it
for
> now, just checking and failing if they are not what we expect.
It's totally accepted that the device might, and often
does, provide different parameters than what was
asked for, hence VCDLinux::AllocateAndStart() works
with a VideoCaptureParams::requested_format.
OnIncoming...Data() allocates precisely what is needed
to wrap incoming size, resolution, format etc. (except
...YuvData() that is limited to YUV420).
Clients down the line are instructed to look at
received VideoFrame parameters and _not_ at the
requested format.
A common mismatch is e.g. asking for 320x240 and getting
320x180 or viceversa.
Obviously BufferTracker's mmap()ed buffers are made
to fit, and that is after VIDIOC_S_FMT.
Am I missing something...?
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.h (right):
https://codereview.chromium.org/967793002/diff/220001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.h:30: static int
TranslatePowerLineFrequencyToV4L2(int frequency);
On 2015/03/13 09:52:53, Pawel Osciak wrote:
> This can be private I think?
Done.
https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin...
File media/video/capture/linux/v4l2_video_capture_delegate.h (right):
https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin...
media/video/capture/linux/v4l2_video_capture_delegate.h:9: #include
<sys/videoio.h>
Let me bring posciak@s comment to the latest patchset:
> This CL may breaking these platforms (may not
> even compile), so there is a real need.
>
Just FI, there are no OpenBSD bots.
> As for preprocessor-filtering, we are already
> using two separate classes, hopefully all
> specifics can go in there. We'd have them in
> separate files and not compile the mplane
> class' file if not supported. The only ifdefs
> in the code would hopefully be in a factory
> method in the base class.
>
> That said, I don't
> know if all splane features are supported in
> videoio.h. I don't know if it still works
> as is right now. And I'm not sure how you'd
> compile-test this if you don't have an OpenBSD
> environment.
>
I think we can safely assume that all SPlane
parts did work, so they will be working the
same, since that functionality hasn't changed
(although it has been cleaned up a bit).
Only clean way to take caera of this is
splitting the 3 classes, hence leaving things
backwards compatible at least.
I found plenty of OS_OPENBSD references in
both gyp files and code, and since it seems
like the right thing to do, to not break
compatibility.
Plus v4l2_video_capture_delegate.{cc,h} was
growing too much anyway. I made patchset 6
in case you're not happy with this
move, case in which we can step back to PS5.
> Perhaps a good idea would be to ask the
> original author who added this and/or on
> chromium-dev.
Well, looking pretty good! I reviewed the 4->5 diff. Just some final remaining nits in there. If you could address them in PS7, I'd take a final look at the split from PS6 in PS7 instead. Thanks! https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:607: buffer->length = num_planes_; On 2015/03/14 03:36:11, mcasas wrote: > On 2015/03/13 09:52:53, Pawel Osciak wrote: > > v4l2_plane_.size() > > Actually it's not that simple. We need to have > an array in order to use it as a pointer > in the very next line (l.688) and that > prevents having length information > into e.g. a ScopedVector. IOW we still need > a member |num_planes_|. Renamed for more > clarity. Oh, I forgot that you were using this as a scoped_ptr<v4l2_plane[]>. But you could as well use std::vector<v4l2_plane> planes_; and then &planes_[0] to get the pointer and planes_.size() to get size. (I'm sure you know that, but std::vector guarantees the content is a plain array if you do &foo[0]. https://chromiumcodereview.appspot.com/967793002/diff/220001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:617: client()->OnIncomingCapturedYuvData(buffer_tracker->GetPlaneStart(0), On 2015/03/14 03:36:11, mcasas wrote: > On 2015/03/13 09:52:52, Pawel Osciak wrote: > > S_FMT may adjust values for plane sizes, resolution and strides. We need to at > > least verify that they will fit within what the client expects in > > OnIncomingCapturedYuvData and fail otherwise. I'm not requiring handling it > for > > now, just checking and failing if they are not what we expect. > > It's totally accepted that the device might, and often > does, provide different parameters than what was > asked for, hence VCDLinux::AllocateAndStart() works > with a VideoCaptureParams::requested_format. > > OnIncoming...Data() allocates precisely what is needed > to wrap incoming size, resolution, format etc. (except > ...YuvData() that is limited to YUV420). > Clients down the line are instructed to look at > received VideoFrame parameters and _not_ at the > requested format. > > A common mismatch is e.g. asking for 320x240 and getting > 320x180 or viceversa. > > Obviously BufferTracker's mmap()ed buffers are made > to fit, and that is after VIDIOC_S_FMT. > > Am I missing something...? > > > My bad, I misread the code, you are actually using the dimensions that S_FMT adjusted for you. Sorry. https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:34: // (depending on |num_planes|). This list is ordered by precedence of use. This suggests a little bit that Splane is for num_planes == 1, and Mplane > 1, while MPLANE does support formats with num_planes = 1. https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:46: // MJPEG is usually sitting fairly low since we don't want to have to decode. This is slightly misleading. We *have* to choose it for higher resolutions due to USB bandwidth limitations. https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:89: // Adds a given mmap()ed plane to internal tracking list. s/internal tracking list/planes_/ https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:168: scoped_ptr<struct v4l2_plane[]> v4l2_planes_; This could be a std::vector<struct v4l2_plane> v4l2_planes_; To get a pointer to array: &v4l2_planes_[0], to get num elems: v4l2_planes_.size(). https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:198: DVLOG(1) << "Unsupported pixel format: " << FourccToString(v4l2_fourcc); We use NOTREACHED above inV4L2VideoCaptureDelegate::GetNumPlanesForFourCc(), but here it's DVLOG(1). Let's make this consistent please (NOTREACHED preferably). https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:352: << "removal, unsupported operation or frequency"; s/flicker removal/frequency/ https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:353: DVLOG_IF(1, retval != EINVAL) << "Mysterious error while setting power " I don't think this is needed. https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:485: FinishFillingV4L2Buffer(&buffer); FillV4L2Buffer() already calls this. https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:549: void* const start =mmap(NULL, buffer.length, PROT_READ | PROT_WRITE, s/=mmap/= mmap/ https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:556: AddMmapedPlane(start, length); Just AddMmappedPlane(start, buffer.length) https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.cc:618: AddMmapedPlane(start, length); AddMmappedPlane(start, buffer.m.planes[p].length); https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... File media/video/capture/linux/v4l2_video_capture_delegate.h (right): https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:9: #include <sys/videoio.h> On 2015/03/14 03:36:12, mcasas wrote: > Let me bring posciak@s comment to the latest patchset: > > > This CL may breaking these platforms (may not > > even compile), so there is a real need. > > > Just FI, there are no OpenBSD bots. I know. That's what I meant, we won't know if we broke that platform (and we would've). > > > As for preprocessor-filtering, we are already > > using two separate classes, hopefully all > > specifics can go in there. We'd have them in > > separate files and not compile the mplane > > class' file if not supported. The only ifdefs > > in the code would hopefully be in a factory > > method in the base class. > > > > That said, I don't > > know if all splane features are supported in > > videoio.h. I don't know if it still works > > as is right now. And I'm not sure how you'd > > compile-test this if you don't have an OpenBSD > > environment. > > > > I think we can safely assume that all SPlane > parts did work, so they will be working the > same, since that functionality hasn't changed > (although it has been cleaned up a bit). > That's a reasonable compromise. I'd still try to contact the original author, or at least put them on cc/review for this CL. > Only clean way to take caera of this is > splitting the 3 classes, hence leaving things > backwards compatible at least. > > I found plenty of OS_OPENBSD references in > both gyp files and code, and since it seems > like the right thing to do, to not break > compatibility. > > Plus v4l2_video_capture_delegate.{cc,h} was > growing too much anyway. I made patchset 6 > in case you're not happy with this > move, case in which we can step back to PS5. > > > Perhaps a good idea would be to ask the > > original author who added this and/or on > > chromium-dev. https://chromiumcodereview.appspot.com/967793002/diff/300001/media/video/capt... media/video/capture/linux/v4l2_video_capture_delegate.h:83: // Request a buffer from V4L2, create and initialize a BufferTracker for it. We also need to update this documentation.
Patchset #7 (id:370001) has been deleted
Patchset #8 (id:410001) has been deleted
Patchset #7 (id:390001) has been deleted
posciak@ PTAL. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:34: // (depending on |num_planes|). This list is ordered by precedence of use. On 2015/03/17 11:05:25, Pawel Osciak wrote: > This suggests a little bit that Splane is for num_planes == 1, and Mplane > 1, > while MPLANE does support formats with num_planes = 1. Rewrote it. I don't want to say too much in this list about the use of planarity. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:46: // MJPEG is usually sitting fairly low since we don't want to have to decode. On 2015/03/17 11:05:25, Pawel Osciak wrote: > This is slightly misleading. We *have* to choose it for higher resolutions due > to USB bandwidth limitations. Done. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:89: // Adds a given mmap()ed plane to internal tracking list. On 2015/03/17 11:05:25, Pawel Osciak wrote: > s/internal tracking list/planes_/ Done. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:168: scoped_ptr<struct v4l2_plane[]> v4l2_planes_; On 2015/03/17 11:05:25, Pawel Osciak wrote: > This could be a std::vector<struct v4l2_plane> v4l2_planes_; > > To get a pointer to array: &v4l2_planes_[0], to get num elems: > v4l2_planes_.size(). Done. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:198: DVLOG(1) << "Unsupported pixel format: " << FourccToString(v4l2_fourcc); On 2015/03/17 11:05:25, Pawel Osciak wrote: > We use NOTREACHED above inV4L2VideoCaptureDelegate::GetNumPlanesForFourCc(), but > here it's DVLOG(1). Let's make this consistent please (NOTREACHED preferably). V4l2FourCcToChromiumPixelFormat() is used during format enumeration from the Factory. In that case, it's OK to find formats not supported. In AllocateAndStart(), return value is dutifully tested for != PIXEL_FORMAT_UNKNOWN. Added a comment in the code. It's also described in the function declaration. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:352: << "removal, unsupported operation or frequency"; On 2015/03/17 11:05:25, Pawel Osciak wrote: > s/flicker removal/frequency/ Done. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:353: DVLOG_IF(1, retval != EINVAL) << "Mysterious error while setting power " On 2015/03/17 11:05:25, Pawel Osciak wrote: > I don't think this is needed. Done. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:485: FinishFillingV4L2Buffer(&buffer); On 2015/03/17 11:05:25, Pawel Osciak wrote: > FillV4L2Buffer() already calls this. Done. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:549: void* const start =mmap(NULL, buffer.length, PROT_READ | PROT_WRITE, On 2015/03/17 11:05:25, Pawel Osciak wrote: > s/=mmap/= mmap/ Done. (Actually already OK on PS7). https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:556: AddMmapedPlane(start, length); On 2015/03/17 11:05:25, Pawel Osciak wrote: > Just AddMmappedPlane(start, buffer.length) Done. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:618: AddMmapedPlane(start, length); On 2015/03/17 11:05:25, Pawel Osciak wrote: > AddMmappedPlane(start, buffer.m.planes[p].length); Done. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.h (right): https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:9: #include <sys/videoio.h> On 2015/03/17 11:05:25, Pawel Osciak wrote: > On 2015/03/14 03:36:12, mcasas wrote: > > Let me bring posciak@s comment to the latest patchset: > > > > > This CL may breaking these platforms (may not > > > even compile), so there is a real need. > > > > > Just FI, there are no OpenBSD bots. > > I know. That's what I meant, we won't know if we broke that platform (and we > would've). > > > > > > As for preprocessor-filtering, we are already > > > using two separate classes, hopefully all > > > specifics can go in there. We'd have them in > > > separate files and not compile the mplane > > > class' file if not supported. The only ifdefs > > > in the code would hopefully be in a factory > > > method in the base class. > > > > > > That said, I don't > > > know if all splane features are supported in > > > videoio.h. I don't know if it still works > > > as is right now. And I'm not sure how you'd > > > compile-test this if you don't have an OpenBSD > > > environment. > > > > > > > I think we can safely assume that all SPlane > > parts did work, so they will be working the > > same, since that functionality hasn't changed > > (although it has been cleaned up a bit). > > > > That's a reasonable compromise. I'd still try to contact the original author, or > at least put them on cc/review for this CL. > > > Only clean way to take caera of this is > > splitting the 3 classes, hence leaving things > > backwards compatible at least. > > > > I found plenty of OS_OPENBSD references in > > both gyp files and code, and since it seems > > like the right thing to do, to not break > > compatibility. > > > > Plus v4l2_video_capture_delegate.{cc,h} was > > growing too much anyway. I made patchset 6 > > in case you're not happy with this > > move, case in which we can step back to PS5. > > > > > Perhaps a good idea would be to ask the > > > original author who added this and/or on > > > chromium-dev. > Chromium-dev should do better since the original CL is from 2011 =-0 [1]. Will add him in Cc: anyway. [1] https://chromium.googlesource.com/chromium/src/+/7aae582cd7730f3d644219148856... https://codereview.chromium.org/967793002/diff/300001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:83: // Request a buffer from V4L2, create and initialize a BufferTracker for it. On 2015/03/17 11:05:25, Pawel Osciak wrote: > We also need to update this documentation. Done.
media/video/capture/linux/ lgtm % nits Perhaps please grab a copy of videoio.h, change your GYP_DEFINES to is_openbsd=1 and try to sanity-check compile? Also, TEST= doesn't say this, but did you sanity-test on a regular USB camera? https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right): https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:52: buffer->m.planes = const_cast<struct v4l2_plane*>(v4l2_planes_.data()); memset to 0 please Also, why do we need this cast? There is a non-const version of data(), and we *will* be changing the memory, so const is not good. https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:59: buffer_tracker->GetPlaneStart(1), This makes me nervous. I'd like to have a check in GetPlaneStart/Length() that we have enough size(). https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:74: mmap(NULL, buffer.m.planes[p].length, PROT_READ | PROT_WRITE, // Some devices require mmap() to be called with both READ and WRITE. // See http://crbug.com/178582. https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... File media/video/capture/linux/v4l2_capture_delegate_multi_plane.h (right): https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.h:48: void SendBuffer(const scoped_refptr<BufferTracker>& buffer_tracker) override; Can this be const?
lgtm % nits https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:251: const size_t u_stride = u_length / frame_format.frame_size.height() / 2; You need parenthesis around height() / 2, now it's wrong by a factor of 4. You should also round up instead of down, so ((...height() + 1) / 2), or use VideoFrame::Rows(kUPlane, I420, frame_format.frame_size.height()) https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... File media/video/capture/linux/v4l2_capture_delegate_multi_plane.h (right): https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.h:42: scoped_refptr<BufferTracker> CreateBufferTracker() override; Shouldn't this be a static function?
mcasas@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@: media/media.gyp/BUILD.gn please
https://codereview.chromium.org/967793002/diff/430001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/967793002/diff/430001/media/BUILD.gn#newcode404 media/BUILD.gn:404: if (is_openbsd) { I think this is always false. https://codereview.chromium.org/967793002/diff/430001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/967793002/diff/430001/media/media.gyp#newcode768 media/media.gyp:768: ['OS=="openbsd"', { Move into else block with above conditional? Do we really still need consideration for OpenBSD?
lgtm if all coments are addressed. https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:90: DLOG_IF(WARNING, (rotation % 90)) nit: dcheck this https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:251: const size_t u_stride = u_length / frame_format.frame_size.height() / 2; On 2015/03/19 18:13:29, magjed wrote: > You need parenthesis around height() / 2, now it's wrong by a factor of 4. You > should also round up instead of down, so ((...height() + 1) / 2), or use > VideoFrame::Rows(kUPlane, I420, frame_format.frame_size.height()) Why dont you have y_stride u_stride etc as input parameters instead of y_length, u_lenth. You donät use y_length anyway. Just the stride, the width and height are in frame_format. https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:261: if (libyuv::I420Copy(y_data, y_stride, So actually holding the buffer in the driver did not work and you need this copy? https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right): https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:61: buffer_tracker->GetPlaneLength(0), I think this should be the stride instead of the plane length. https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.h (right): https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:65: return static_cast<uint8_t* const>(planes_[plane].start); why is start a void ptr if you want a uint ptr?
Patchset #8 (id:450001) has been deleted
dalecurtis@ PTAL. (posciak@ I try with SPlane Webcams all the time, see updated TEST= section). https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:90: DLOG_IF(WARNING, (rotation % 90)) On 2015/03/19 20:38:17, perkj wrote: > nit: dcheck this Done. https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:251: const size_t u_stride = u_length / frame_format.frame_size.height() / 2; On 2015/03/19 18:13:29, magjed wrote: > You need parenthesis around height() / 2, now it's wrong by a factor of 4. You > should also round up instead of down, so ((...height() + 1) / 2), or use > VideoFrame::Rows(kUPlane, I420, frame_format.frame_size.height()) Done. https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:251: const size_t u_stride = u_length / frame_format.frame_size.height() / 2; On 2015/03/19 20:38:17, perkj wrote: > On 2015/03/19 18:13:29, magjed wrote: > > You need parenthesis around height() / 2, now it's wrong by a factor of 4. You > > should also round up instead of down, so ((...height() + 1) / 2), or use > > VideoFrame::Rows(kUPlane, I420, frame_format.frame_size.height()) > > Why dont you have y_stride u_stride etc as input parameters instead of y_length, > u_lenth. You donät use y_length anyway. Just the stride, the width and height > are in frame_format. Done. https://codereview.chromium.org/967793002/diff/430001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:261: if (libyuv::I420Copy(y_data, y_stride, On 2015/03/19 20:38:17, perkj wrote: > So actually holding the buffer in the driver did not work and you need this > copy? After much discussion with posciak@ we decided not to change the synchronicity of VCD::Client, IOW, VCD expects the Client to have fully used and released the |y_data|, |u_data|, |v_data| after calling here. So we have to make a copy. Capturing directly on Buffer.data() or on a mapped dma-buf would need revisiting the VCD::Client, so we can release resources in a Callback, i.e. asynchronously, and we thought better to address that in a later CL. https://codereview.chromium.org/967793002/diff/430001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/967793002/diff/430001/media/BUILD.gn#newcode404 media/BUILD.gn:404: if (is_openbsd) { On 2015/03/19 19:07:16, DaleCurtis wrote: > I think this is always false. There are a number of specifics in media.gyp for this platform [1], but perhaps you are talking about the is_openbsd=false in [2]? However in media we have at least another if(is_openbsd) ... [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/media.gyp&sq... [2] https://code.google.com/p/chromium/codesearch#chromium/src/media/media_option... [3] https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/BUILD.... https://codereview.chromium.org/967793002/diff/430001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/967793002/diff/430001/media/media.gyp#newcode768 media/media.gyp:768: ['OS=="openbsd"', { On 2015/03/19 19:07:17, DaleCurtis wrote: > Move into else block with above conditional? Done. > Do we really still need consideration for OpenBSD? We discussed it a couple of iterations before. While we have no *BSD bots per se, it's compiling and running right now, so it'd be nice to break the compatibility. However, *BSD does not support MPlane API, hence the extra provisions. On the other hand lke I said elsewhere, there are OpenBSD specific files already [1]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/media.gyp&sq... https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right): https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:52: buffer->m.planes = const_cast<struct v4l2_plane*>(v4l2_planes_.data()); On 2015/03/19 07:52:43, Pawel Osciak wrote: > memset to 0 please Done. > Also, why do we need this cast? There is a non-const version of data(), and we > *will* be changing the memory, so const is not good. The compiler saw that the method is marked |const| and used: const T* data() const; const_cast is a way around, since it eliminates the constness. Alternatively, I marked |v4l2_planes_| as mutable, since is a scratchpad and can be used in const methods without further ado. As a side bonus, this mutable allows for FillV4L2Format() to be marked const as well. https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:59: buffer_tracker->GetPlaneStart(1), On 2015/03/19 07:52:43, Pawel Osciak wrote: > This makes me nervous. I'd like to have a check in GetPlaneStart/Length() that > we have enough size(). Done. DCHECK is good for me, but please speak up otherwise. https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:61: buffer_tracker->GetPlaneLength(0), On 2015/03/19 20:38:17, perkj wrote: > I think this should be the stride instead of the plane length. Done. https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:74: mmap(NULL, buffer.m.planes[p].length, PROT_READ | PROT_WRITE, On 2015/03/19 07:52:43, Pawel Osciak wrote: > // Some devices require mmap() to be called with both READ and WRITE. > // See http://crbug.com/178582. Done. https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... File media/video/capture/linux/v4l2_capture_delegate_multi_plane.h (right): https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.h:42: scoped_refptr<BufferTracker> CreateBufferTracker() override; On 2015/03/19 18:13:29, magjed wrote: > Shouldn't this be a static function? I would like to but they I could not override it. https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.h:48: void SendBuffer(const scoped_refptr<BufferTracker>& buffer_tracker) override; On 2015/03/19 07:52:43, Pawel Osciak wrote: > Can this be const? Done. https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.h (right): https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.h:65: return static_cast<uint8_t* const>(planes_[plane].start); On 2015/03/19 20:38:17, perkj wrote: > why is start a void ptr if you want a uint ptr? mmap() returns a void*. I think it's easier to do the static_cast<uint8_t*>() on Plane construction, changed.
https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right): https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:52: buffer->m.planes = const_cast<struct v4l2_plane*>(v4l2_planes_.data()); On 2015/03/19 22:57:00, mcasas wrote: > On 2015/03/19 07:52:43, Pawel Osciak wrote: > > memset to 0 please > > Done. > > > Also, why do we need this cast? There is a non-const version of data(), and we > > *will* be changing the memory, so const is not good. > > The compiler saw that the method is marked |const| > and used: > const T* data() const; The method should not be const? https://codereview.chromium.org/967793002/diff/430001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:59: buffer_tracker->GetPlaneStart(1), On 2015/03/19 22:57:00, mcasas wrote: > On 2015/03/19 07:52:43, Pawel Osciak wrote: > > This makes me nervous. I'd like to have a check in GetPlaneStart/Length() that > > we have enough size(). > > Done. DCHECK is good for me, but please speak up > otherwise. I'd prefer something stronger please, better than invalid memory access issues. https://codereview.chromium.org/967793002/diff/470001/media/video/capture/lin... File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right): https://codereview.chromium.org/967793002/diff/470001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:62: buffer_tracker->GetPlaneLength(0) / capture_format().frame_size.height(); If you need stride, then you should use bytesperline from the v4l2_pix_format_mplane struct. And have some kind of dcheck that it's what you expect. https://codereview.chromium.org/967793002/diff/470001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:70: y_stride, The prototype of VideoCaptureDevice::Client::OnIncomingCapturedYuvData() has lengths. https://codereview.chromium.org/967793002/diff/490001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://codereview.chromium.org/967793002/diff/490001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:284: DCHECK_LE(r_buffer.count, kNumVideoBuffers); count may be larger than what we requested and that should be fine. kNumVideoBuffers is not Max https://codereview.chromium.org/967793002/diff/490001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/967793002/diff/490001/media/video/capture/vid... media/video/capture/video_capture_device.h:213: size_t y_length, This is stride?
Patchset #10 (id:510001) has been deleted
Patchset #10 (id:530001) has been deleted
Patchset #10 (id:550001) has been deleted
posciak@ PTAL. As spoken offline, there was action in SendBuffer()s, V4L2CaptureDelegate holds on to v4l2_format video_fmt_; and passes it to SendBuffer() for using it as buffer length or plane stride, respectively. https://codereview.chromium.org/967793002/diff/470001/media/video/capture/lin... File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right): https://codereview.chromium.org/967793002/diff/470001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:62: buffer_tracker->GetPlaneLength(0) / capture_format().frame_size.height(); On 2015/03/20 00:56:06, Pawel Osciak wrote: > If you need stride, then you should use bytesperline from the > v4l2_pix_format_mplane struct. And have some kind of dcheck that it's what you > expect. Done. https://codereview.chromium.org/967793002/diff/470001/media/video/capture/lin... media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:70: y_stride, On 2015/03/20 00:56:06, Pawel Osciak wrote: > The prototype of VideoCaptureDevice::Client::OnIncomingCapturedYuvData() has > lengths. Done. https://codereview.chromium.org/967793002/diff/490001/media/video/capture/lin... File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://codereview.chromium.org/967793002/diff/490001/media/video/capture/lin... media/video/capture/linux/v4l2_video_capture_delegate.cc:284: DCHECK_LE(r_buffer.count, kNumVideoBuffers); On 2015/03/20 00:56:07, Pawel Osciak wrote: > count may be larger than what we requested and that should be fine. > kNumVideoBuffers is not Max Removed. https://codereview.chromium.org/967793002/diff/490001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/967793002/diff/490001/media/video/capture/vid... media/video/capture/video_capture_device.h:213: size_t y_length, On 2015/03/20 00:56:07, Pawel Osciak wrote: > This is stride? Done. Gosh this signature is repeated in 7 places X-|
https://codereview.chromium.org/967793002/diff/570001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/967793002/diff/570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:241: DCHECK_GE(u_stride, 1u * frame_format.frame_size.width() / 2); It would still be nice to have height + 1 here. For example, a 31x31 image needs u/v strides at least 16, not 15. https://codereview.chromium.org/967793002/diff/570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:262: dst_y, y_stride, The destination strides are wrong. You calculate |dst_u| and |dst_v| using VideoFrame::PlaneAllocationSize which implies tightly packed strides, but you use the src strides here.
Patchset #11 (id:590001) has been deleted
lgtm
Patchset #11 (id:610001) has been deleted
I'm not sure openbsd support even compiles anymore, certainly ffmpeg doesn't --
but lgtm for medio.{gyp,gn}
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org, posciak@chromium.org, magjed@chromium.org Link to the patchset: https://codereview.chromium.org/967793002/#ps630001 (title: "magjed@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967793002/630001
Message was sent while issue was closed.
Committed patchset #11 (id:630001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d3d37d0f3493dd28cd687fe9285b0aac81a61dae Cr-Commit-Position: refs/heads/master@{#321612}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:630001) has been created in https://codereview.chromium.org/1031583002/ by perkj@chromium.org. The reason for reverting is: Seems to breaks bots that use real cameras: https://build.chromium.org/p/chromium.webrtc/builders/Linux%20Tester/builds/136 WebRtcPerfBrowserTest.MANUAL_RunsAudioVideoCall60SecsAndLogsInternalMetrics WebRtcPerfBrowserTest.MANUAL_RunsOneWayCall60SecsAndLogsInternalMetrics.
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:630001) has been created in https://codereview.chromium.org/1024093003/ by phoglund@chromium.org. The reason for reverting is: Breaks the WebRTC video quality test and other tests using the fake device: [20487:20512:0323/021308:FATAL:video_capture_device.h(151)] Check failed: capture_api_type_ != API_TYPE_UNKNOWN (2 vs. 2) This happens on Linux only, 100% reproducible: https://uberchromegw.corp.google.com/i/chromium.webrtc/builders/Linux%20Teste....
Message was sent while issue was closed.
On 2015/03/23 09:38:12, phoglund wrote: > A revert of this CL (patchset #11 id:630001) has been created in > https://codereview.chromium.org/1024093003/ by mailto:phoglund@chromium.org. > > The reason for reverting is: Breaks the WebRTC video quality test and other > tests using the fake device: > > [20487:20512:0323/021308:FATAL:video_capture_device.h(151)] Check failed: > capture_api_type_ != API_TYPE_UNKNOWN (2 vs. 2) > > This happens on Linux only, 100% reproducible: > https://uberchromegw.corp.google.com/i/chromium.webrtc/builders/Linux%20Teste.... Thanks Patrick! https://codereview.chromium.org/1026073002/ adds the fix and relands.
Message was sent while issue was closed.
Old comments. Ignore. https://codereview.chromium.org/967793002/diff/570001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/967793002/diff/570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:241: DCHECK_GE(u_stride, 1u * frame_format.frame_size.width() / 2); On 2015/03/20 07:53:45, magjed wrote: > It would still be nice to have height + 1 here. For example, a 31x31 image needs > u/v strides at least 16, not 15. Done. https://codereview.chromium.org/967793002/diff/570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:262: dst_y, y_stride, On 2015/03/20 07:53:45, magjed wrote: > The destination strides are wrong. You calculate |dst_u| and |dst_v| using > VideoFrame::PlaneAllocationSize which implies tightly packed strides, but you > use the src strides here. Changed to using those from VideoFrame::RowBytes(). |
