Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2700173002:
Add MJPEG support to FakeVideoCaptureDevice (Closed)
|
Created:
3 years, 10 months ago by chfremer Modified:
3 years, 10 months ago CC:
chromium-reviews, posciak+watch_chromium.org, chfremer+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd MJPEG support to FakeVideoCaptureDevice
Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us
to expand our test coverage of the video capture pipeline.
This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL12.
BUG=584797
TEST=
capture_unittests --gtest_filter="*Video*"
[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
Review-Url: https://codereview.chromium.org/2700173002
Cr-Commit-Position: refs/heads/master@{#452674}
Committed: https://chromium.googlesource.com/chromium/src/+/98985be5a81f6862fa536c05cda2292a36f18e8b
Patch Set 1 #
Total comments: 25
Patch Set 2 : mcasas@ suggestions #Patch Set 3 : rebase and format #Patch Set 4 : Fix for compilers on Win and Android #
Total comments: 14
Patch Set 5 : mcasas@ suggestions #
Total comments: 1
Patch Set 6 : Add a const #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 37 (27 generated)
Description was changed from ========== Add MJPEG support to FakeVideoCaptureDevice BUG= ========== to ========== Add MJPEG support to FakeVideoCaptureDevice Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us to expand our test coverage of the video capture pipeline. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL12. BUG=584797 TEST= capture_unittests --gtest_filter="*Video*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ==========
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chfremer@chromium.org changed reviewers: + emircan@chromium.org, guidou@chromium.org, mcasas@chromium.org
chfremer@chromium.org changed required reviewers: + mcasas@chromium.org
mcasas@: PTAL emircan@: FYI guidou@: FYI
https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:175: base::TimeTicks first_ref_time_; I forgot to remove members |device_state|, |client_| and |first_ref_time_| from here, since they have moved up to the base class. Will remove in PS2.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:275: } I find this part hard to read, mostly because of the long lines. What about: const VideoPixelFormat painter_pixel_format = pixel_format == PIXEL_FORMAT_MJPEG ? PIXEL_FORMAT_ARGB : pixel_format; video_frame_painter = base::MakeUnique<PacmanFramePainter>( painter_pixel_format, device_state.get()); https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:293: } Maybe not in this CL, but I don't see any fundamental reason why we could not produce a MJPEG encoded stream with client buffers; can we just add a TODO() (with an https://crbug.com/...) to add support for that some time in the future? (mark the bug as GoodFirstBug). https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:573: device_state_ = nullptr; l.567-568 and l.572-573 are common among all FrameDeliverers, is it worth moving those common parts to the base class ? (I'd say yes because we'd avoid entirely the implementations for e.g. JpegEncodingFrameDeliverer::Initialize(), ClientBufferFrameDeliverer::Initialize() and Uninitialize(), and simplify the others) https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:590: } Instead of having a |argb_buffer_| and |argb_buffer_size_in_bytes_|, just have an std::vector argb_buffer_ and here do argb_buffer_.resize(VideoFrame::AllocationSize( PIXEL_FORMAT_ARGB, device_state_->format.frame_size)); which is a no-op if the size is the same as the current. And ISO |argb_buffer_size_in_bytes_| just use argb_buffer_.size(). https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:594: static const int kBytePerPixel = 4; s/kBytePer/BytesPer/ But you can skip it altogether, see my comments below. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:597: argb_buffer_.get(), gfx::JPEGCodec::FORMAT_RGBA, We seem to be using ARGB format internally whereas here RGBA is selected, wouldn't that be a problem? Best is to use skia byte endianness selection, emircan@ knows. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:600: device_state_->format.frame_size.width() * kBytePerPixel, kQuality, Use here VideoFrame::RowBytes(1 /* plane */, PIXEL_FORMAT_ARGB,device_state_->format.frame_size.width()) to calculate the #bytes per row. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:44: } Yeah, make this again file static. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.h (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.h:27: static media::VideoPixelFormat GetPixelFormatFromDeviceId( Should be private, but better than that just keep it static in fake_video_capture_device_factory.cc as it was. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_unittest.cc:281: } If you add a TODO to add support for Client buffers+mjpe, add a corresponding TODO to remove this bail-out provision.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL Incorporated suggestions. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:175: base::TimeTicks first_ref_time_; On 2017/02/17 18:07:38, chfremer wrote: > I forgot to remove members |device_state|, |client_| and |first_ref_time_| from > here, since they have moved up to the base class. > Will remove in PS2. Done. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:275: } On 2017/02/17 18:35:47, mcasas wrote: > I find this part hard to read, mostly because of the long lines. > What about: > > const VideoPixelFormat painter_pixel_format = > pixel_format == PIXEL_FORMAT_MJPEG ? PIXEL_FORMAT_ARGB : pixel_format; > > video_frame_painter = base::MakeUnique<PacmanFramePainter>( > painter_pixel_format, device_state.get()); > Done. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:293: } On 2017/02/17 18:35:47, mcasas wrote: > Maybe not in this CL, but I don't see any fundamental > reason why we could not produce a MJPEG encoded > stream with client buffers; can we just add a TODO() > (with an https://crbug.com/...) to add support for that > some time in the future? (mark the bug as GoodFirstBug). I thought so, too, until I tried and ran into some issues. One issue is that the size in bytes required by the jpeg frame depends on the content, and therefore varies with each frame. To accomodate this, the API of gfx::JPEGCodec::Encode() takes a pointer to a std::vector<> which it resizes as needed. Also, the API of VideoCaptureDevice::Client does not currently allow us to specify how large we want a buffer to be in bytes. It only allows us to specify a resolution and format, but there is no way it can properly estimate how large a buffer would need to be for MJPEG encoding. With that in mind, it seems that adding support for this case would require some serious design and refactoring work. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:573: device_state_ = nullptr; On 2017/02/17 18:35:47, mcasas wrote: > l.567-568 and l.572-573 are common among all > FrameDeliverers, is it worth moving those common > parts to the base class ? (I'd say yes because we'd > avoid entirely the implementations for e.g. > JpegEncodingFrameDeliverer::Initialize(), > ClientBufferFrameDeliverer::Initialize() and > Uninitialize(), and simplify the others) Agreed. Thanks. I moved the common initialize and uninitialize logic up to the base class and added template methods for the derived classes to do custom initialize and uninitialize logic if they need it. This also enabled encapsulating the members of the base class by making them private. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:590: } On 2017/02/17 18:35:47, mcasas wrote: > Instead of having a |argb_buffer_| and > |argb_buffer_size_in_bytes_|, just have an > > std::vector argb_buffer_ > > and here do > > argb_buffer_.resize(VideoFrame::AllocationSize( > PIXEL_FORMAT_ARGB, device_state_->format.frame_size)); > > which is a no-op if the size is the same as the current. > And ISO |argb_buffer_size_in_bytes_| just use > argb_buffer_.size(). > > Nice. Thanks. Done. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:594: static const int kBytePerPixel = 4; On 2017/02/17 18:35:47, mcasas wrote: > s/kBytePer/BytesPer/ > But you can skip it altogether, see my comments below. Done. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:597: argb_buffer_.get(), gfx::JPEGCodec::FORMAT_RGBA, On 2017/02/17 18:35:47, mcasas wrote: > We seem to be using ARGB format internally whereas > here RGBA is selected, wouldn't that be a problem? > Best is to use skia byte endianness selection, emircan@ > knows. Thanks for raising this. Yes, this probably leads to the colors being incorrect, at least on some platforms. I initially followed the code from the TakePhoto() method, which does the same just with PNG instead of JPG, and then forgot to follow up on it. In both cases, the frame is actually painted using kN32_SkColorType, which is either RGBA or BGRA, depending on the platform. The fact that PIXEL_FORMAT_ARGB is used to request this is super confusing. Ideally, for both PNG and JPG, we would paint in RGB, independent of platform. See proposal in next patch set. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:600: device_state_->format.frame_size.width() * kBytePerPixel, kQuality, On 2017/02/17 18:35:47, mcasas wrote: > Use here > > VideoFrame::RowBytes(1 /* plane */, > PIXEL_FORMAT_ARGB,device_state_->format.frame_size.width()) > > to calculate the #bytes per row. Did you mean 0 instead of 1 for the plane index? Done. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:44: } On 2017/02/17 18:35:47, mcasas wrote: > Yeah, make this again file static. See other comment. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.h (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.h:27: static media::VideoPixelFormat GetPixelFormatFromDeviceId( On 2017/02/17 18:35:47, mcasas wrote: > Should be private, but better than that just keep > it static in fake_video_capture_device_factory.cc > as it was. The reason I am exposing this here is because the test FakeVideoCaptureDeviceTest.GetDeviceSupportedFormats needs this to know what pixel format to expect. If we are not exposing this here, we would have to hard-code the expectation in the test, which would mean duplication. https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_unittest.cc:281: } On 2017/02/17 18:35:47, mcasas wrote: > If you add a TODO to add support for Client > buffers+mjpe, add a corresponding TODO to > remove this bail-out provision. Currently not planning a TODO. See my other comment as for why.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.h (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.h:27: static media::VideoPixelFormat GetPixelFormatFromDeviceId( On 2017/02/22 17:28:42, chfremer wrote: > On 2017/02/17 18:35:47, mcasas wrote: > > Should be private, but better than that just keep > > it static in fake_video_capture_device_factory.cc > > as it was. > > The reason I am exposing this here is because the test > FakeVideoCaptureDeviceTest.GetDeviceSupportedFormats needs this to know what > pixel format to expect. If we are not exposing this here, we would have to > hard-code the expectation in the test, which would mean duplication. Duplication for a test is OK, otherwise we'd be comparing the implementation against itself, and it would always work :-) Also, I'd like tests to fail when someone comes along and modifies e.g. the pixel format of a given device entry. That way they'll have to update it. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:124: bool has_client() { return client_ != nullptr; } bool HasClient() const { return !client_; } but IMHO is not needed, just use !client() in the call sites https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:141: void UninitializeDerived() override; There's no need for these ...Derived() guys, instead: class FrameDeliverer { pubic: virtual void Initialize(...) { ...} }; class OwnBufferFrameDeliverer { public: void Initialize() override { FrameDeliverer::Initialize(...); // specific stuff here } }; I like this call-super much better, but MFowler doesn't [1]. So if you still prefer the override-ways, then please substitute "Derived" for something meaningful (like, e.g. "Buffers"?). [1] https://www.martinfowler.com/bliki/CallSuper.html https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:256: // reaching end of non-void method. This is a widespread pattern in Chromium, no need to add a comment for this. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:355: case Format::RGB: I'd advice against touching code in CLs where you mostly add new stuff. Also, in the vast majority of cases, RGB32s are more performant than RGB24 format, which is hard to work with due to the strange alignment (in many cases, such as Windows, RGB24 is implemented as RGB32 ignoring one component) and the multiplications by 3 (as opposed to 4 where compilers will insert fast SIMD-shifts). So: please leave the ARGB parts in place, here and elsewhere. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:431: } I'm not sure this pixel swizzling is clearer than what was before, where we could at least see the paint.setARGB(255, 0, 127, 0); Can we do all this movements in any other way? Maybe we can use some nice libyuv function. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:566: JpegEncodingFrameDeliverer::JpegEncodingFrameDeliverer( JpegEncodingFrameDeliverer is declared after OwnBufferFrameDeliverer and ClientBufferFrameDeliverer, but here the definition goes before. Make it consistent. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.h (right): https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.h:30: PixelFormat format); We already have a |VideoPixelFormat|. I think you want to limit the ones accepted/supported by this particular class (and by PacmanFramePainter, where we have another redefinition). Find other way, e.g. DCHECKs, otherwise the benefits of compile-time checking that only supported formats are used, are overriden by the extra code you need to translate back and forth.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.h (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.h:27: static media::VideoPixelFormat GetPixelFormatFromDeviceId( On 2017/02/22 20:26:32, mcasas wrote: > On 2017/02/22 17:28:42, chfremer wrote: > > On 2017/02/17 18:35:47, mcasas wrote: > > > Should be private, but better than that just keep > > > it static in fake_video_capture_device_factory.cc > > > as it was. > > > > The reason I am exposing this here is because the test > > FakeVideoCaptureDeviceTest.GetDeviceSupportedFormats needs this to know what > > pixel format to expect. If we are not exposing this here, we would have to > > hard-code the expectation in the test, which would mean duplication. > > Duplication for a test is OK, otherwise we'd be comparing > the implementation against itself, and it would always > work :-) > Also, I'd like tests to fail when someone comes along and > modifies e.g. the pixel format of a given device entry. That > way they'll have to update it. Duplication for a test can be problematic. We want to avoid testing implementations by providing and verifying against an equivalent (duplicate) second implementation. In this case, the duplicated implementation would be this simple mapping from device id strings to VideoPixelFormat. As you pointed out, by exposing this static method and using it in the test, we exclude it from test coverage. And this is okay, because it is so basic that the only way to test it would be to compare it to a duplicate. Why is it an advantage to have tests fail when someone wants to modify the pixel format of a given device entry? The way I see it, the only thing this does is making it slightly harder for the person making the change by requiring the same change in two locations (a bit like a confirmation dialog asking "are you sure?"). Anyway, since the cost of the duplication is not high in this case, I followed your suggestion in the latest patch set. Done. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:124: bool has_client() { return client_ != nullptr; } On 2017/02/22 20:26:33, mcasas wrote: > bool HasClient() const { return !client_; } > > but IMHO is not needed, just use !client() > in the call sites Done. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:141: void UninitializeDerived() override; On 2017/02/22 20:26:32, mcasas wrote: > There's no need for these ...Derived() guys, instead: > > class FrameDeliverer { > pubic: > virtual void Initialize(...) { ...} > }; > > class OwnBufferFrameDeliverer { > public: > void Initialize() override { > FrameDeliverer::Initialize(...); > // specific stuff here > } > }; > > I like this call-super much better, but MFowler doesn't [1]. > So if you still prefer the override-ways, then please > substitute "Derived" for something meaningful (like, e.g. > "Buffers"?). > > [1] https://www.martinfowler.com/bliki/CallSuper.html I am intentionally avoiding requiring the call to super. I agree that the ...Derived is a desparate solution for a name. The reason I ended up choosing this name is, because I wanted to avoid the base class knowing or assuming anything about the implementation of the subclasses. Naming the template method InitializeBuffers() would imply that the base class knows that all subclasses involve some buffers they want to initialize when this is called. I guess I got stuck half way between using inheritance and composition. Since you seem to prefer using inheritance here, I'll go with your suggestion. Done. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:256: // reaching end of non-void method. On 2017/02/22 20:26:33, mcasas wrote: > This is a widespread pattern in Chromium, no > need to add a comment for this. Done. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:355: case Format::RGB: On 2017/02/22 20:26:33, mcasas wrote: > I'd advice against touching code in CLs where you > mostly add new stuff. Also, in the vast majority of > cases, RGB32s are more performant than RGB24 > format, which is hard to work with due to the strange > alignment (in many cases, such as Windows, RGB24 > is implemented as RGB32 ignoring one component) > and the multiplications by 3 (as opposed to 4 where > compilers will insert fast SIMD-shifts). > So: please leave the ARGB parts in place, here and > elsewhere. Agreed that mixing new stuff with changing old stuff is not a great idea. I was hoping to offer a resolution to your comment on the earlier patchset pointing out the mismatch between the format expected by the encoder for JPG (RGBA) and the format delivered by the painter (RGBA or BGRA depending on the platform). Since SkBitmap does not support cross-platform painting in RGBA I thought this would be viable solution. But it seems like the encoders support both RGBA and BGRA, so for the latest patch set I reverted back to painting in Sk_N32 and then telling the encoders which source format to use. Done. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:431: } On 2017/02/22 20:26:33, mcasas wrote: > I'm not sure this pixel swizzling is clearer than what was before, > where we could at least see the > paint.setARGB(255, 0, 127, 0); > Can we do all this movements in any other way? Maybe > we can use some nice libyuv function. Definitely not clearer. Removed. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:566: JpegEncodingFrameDeliverer::JpegEncodingFrameDeliverer( On 2017/02/22 20:26:33, mcasas wrote: > JpegEncodingFrameDeliverer is declared after > OwnBufferFrameDeliverer and ClientBufferFrameDeliverer, > but here the definition goes before. Make it consistent. Done. https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.h (right): https://codereview.chromium.org/2700173002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.h:30: PixelFormat format); On 2017/02/22 20:26:33, mcasas wrote: > We already have a |VideoPixelFormat|. I think you want > to limit the ones accepted/supported by this particular > class (and by PacmanFramePainter, where we have > another redefinition). Find other way, e.g. DCHECKs, > otherwise the benefits of compile-time checking that > only supported formats are used, are overriden by the > extra code you need to translate back and forth. You are right. The reason I added these enum classes is as a C++ way to express that only a certain subset of formats is supported. I feel that doing so is much better than expressing the same using runtime checks and comments. I don't see how the need to translate back and forth negates this. In the new patch set, I am changing one direction of the translation from an explicit mapping to a static_cast. Please let me know if that eases (or increases?) the concern about the translation. When you recommend trying to find a solution using DCHECKs, is there some alternative you see that does not boil down to runtime checks and comments?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ps5 lgtm https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.h (right): https://codereview.chromium.org/2700173002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.h:27: static media::VideoPixelFormat GetPixelFormatFromDeviceId( On 2017/02/23 01:14:27, chfremer wrote: > On 2017/02/22 20:26:32, mcasas wrote: > > On 2017/02/22 17:28:42, chfremer wrote: > > > On 2017/02/17 18:35:47, mcasas wrote: > > > > Should be private, but better than that just keep > > > > it static in fake_video_capture_device_factory.cc > > > > as it was. > > > > > > The reason I am exposing this here is because the test > > > FakeVideoCaptureDeviceTest.GetDeviceSupportedFormats needs this to know what > > > pixel format to expect. If we are not exposing this here, we would have to > > > hard-code the expectation in the test, which would mean duplication. > > > > Duplication for a test is OK, otherwise we'd be comparing > > the implementation against itself, and it would always > > work :-) > > Also, I'd like tests to fail when someone comes along and > > modifies e.g. the pixel format of a given device entry. That > > way they'll have to update it. > > Duplication for a test can be problematic. We want to avoid testing > implementations by providing and verifying against an equivalent (duplicate) > second implementation. In this case, the duplicated implementation would be this > simple mapping from device id strings to VideoPixelFormat. As you pointed out, > by exposing this static method and using it in the test, we exclude it from test > coverage. And this is okay, because it is so basic that the only way to test it > would be to compare it to a duplicate. > > Why is it an advantage to have tests fail when someone wants to modify the pixel > format of a given device entry? The way I see it, the only thing this does is > making it slightly harder for the person making the change by requiring the same > change in two locations (a bit like a confirmation dialog asking "are you > sure?"). I like this "are you sure" :-) and I'm not the only one since such dialogs are pervasive... think that this code is probably going to be modified down the line by other developers, and seeing tests failing, and forcing them to update those accordingly, provides an extra check (a CL with no tests updated means... that there was no test coverage?). > > Anyway, since the cost of the duplication is not high in this case, I followed > your suggestion in the latest patch set. > > Done. https://codereview.chromium.org/2700173002/diff/80001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2700173002/diff/80001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:340: VideoPixelFormat expected_format_by_device_index[] = { nit: const
The CQ bit was checked by chfremer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2700173002/#ps100001 (title: "Add a const")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487888113644880,
"parent_rev": "ea07a32dac283aab397f035a4d3e8c40debe6280", "commit_rev":
"98985be5a81f6862fa536c05cda2292a36f18e8b"}
Message was sent while issue was closed.
Description was changed from ========== Add MJPEG support to FakeVideoCaptureDevice Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us to expand our test coverage of the video capture pipeline. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL12. BUG=584797 TEST= capture_unittests --gtest_filter="*Video*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ========== to ========== Add MJPEG support to FakeVideoCaptureDevice Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us to expand our test coverage of the video capture pipeline. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL12. BUG=584797 TEST= capture_unittests --gtest_filter="*Video*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... Review-Url: https://codereview.chromium.org/2700173002 Cr-Commit-Position: refs/heads/master@{#452674} Committed: https://chromium.googlesource.com/chromium/src/+/98985be5a81f6862fa536c05cda2... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/98985be5a81f6862fa536c05cda2...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2712123003/ by alemate@chromium.org. The reason for reverting is: Speculative revert because of video_ChromeRTCHWDecodeUsed failure on PFQ informational builders: [4316:4316:0224/131423.612615:ERROR:v4l2_jpeg_decode_accelerator.cc(206)] Failed to open device. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
