Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2447233002:
FakeVideoCaptureDevice: Y16 testing support. (Closed)
|
Created:
4 years, 1 month ago by aleksandar.stojiljkovic Modified:
4 years, 1 month ago Reviewers:
mcasas CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, xjz+watch_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFakeVideoCaptureDevice: Y16 testing support.
Adds:
- support for Y16 buffer format.
- support for defining number of devices from command line.
- generated color control points in corners that could be used to verify the
frame rendering with no need for the reference render bitmap.
BUG=624436
Committed: https://crrev.com/38481bca66b6ca654bbe73cf6f0c418621d517cc
Cr-Commit-Position: refs/heads/master@{#428001}
Patch Set 1 #
Total comments: 24
Patch Set 2 : After calling git cl format media. #Patch Set 3 : review #8 fixes. Thanks mcasas@. #
Total comments: 2
Patch Set 4 : Nits. #
Messages
Total messages: 31 (17 generated)
The CQ bit was checked by aleksandar.stojiljkovic@intel.com 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...
Description was changed from ========== FakeVideoCaptureDevice: Y16 testing support. Adds: - support for Y16 buffer format. - support for defining number of devices from command line. - generated color control points in corners that can beused to verify the frame rendering with no need for the reference render bitmap. BUG=624436 ========== to ========== FakeVideoCaptureDevice: Y16 testing support. Adds: - support for Y16 buffer format. - support for defining number of devices from command line. - generated color control points in corners that could be used to verify the frame rendering with no need for the reference render bitmap. BUG=624436 ==========
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...)
aleksandar.stojiljkovic@intel.com changed reviewers: + mcasas@chromium.org
On 2016/10/24 19:48:47, mcasas wrote https://codereview.chromium.org/2428263004/ ... > Also, what if we review separately the changes to > FakeVCD & relatives? I imagine they are needed for > content_{browser,unit}tests, but those could be > reviewed separately as well. That'll probably get your > CLs landed faster :-) mcasas@chromium.org. Thanks. The patch is split here and covered other suggestions you had: - reverted FakeVideoCaptureDeviceBase - added sub argument to define number of devices (still left that second is always Y16).
Looking good, couple of comments and a few suggestion/Chromiumisms. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:51: gfx::Point squares[] = {{0, 0}, const ? Also |start| in the next line. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:58: for (unsigned i = 0; i < sizeof(squares) / sizeof(gfx::Point); ++i) { Suggestion: for (const auto& corner : squares) { for (int y = corner.y(); y < corner.y() + side; ++y) { for (int x = corner.x(); y < corner.x() + side; ++x) { // same here https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:61: unsigned value = const unsigned int? https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:146: } No need for {} in one-line bodies. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:148: DrawGradientSquares(frame_format, data, elapsed_time, frame_size); So, just to make sure, I think you mentioned elsewhere that this is not going to throw off tests that expect some green pixels on pixels in the upper left corner or its vicinity, correct? https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:218: << "buffers"; nit: whitespace here before "buffers" and after "own" in l.217. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:21: return media::PIXEL_FORMAT_I420; Suggestion: send the device index (|n| in l.49 media::VideoPixelFormat GetPixelFormat(int device_index) { return device_index == 1 ? media::PIXEL_FORMAT_Y16 : media::PIXEL_FORMAT_I420; } Note also the CamelCase naming for methods/functions. This will also affect l.37. A larger comment is that if #devices is 2, then we get an I420 and an Y16; but with the current code, if we select |device-count = 4| then we don't get two "couples" I420-Y16, but more I420s. Is this intended/unintended/don't care ... ? https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:135: unsigned count = 0; unsigned int? I think it's uncommon to see just |unsigned| in Cr codebase. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:140: static_cast<int>(number_of_devices_)); One line less if we do: number_of_devices_ = std::min(kFakeCaptureMaxDeviceCount, std::max(kFakeCaptureMinDeviceCount, static_cast<int>(count))); https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_unittest.cc:191: return client; Suggestion: return base::MakeUnique<MockClient>(base::Bind( &FakeVideoCaptureDeviceBase::OnFrameCaptured, base::Unretained(this))); https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_unittest.cc:280: ASSERT_EQ(descriptors->size(), 3u); ASSERT_EQ(expected, actual), so here: ASSERT_EQ(3u, descriptors->size()); I'm afraid the comparisons below are also reversed (which is not your fault).
Patch Set 3 : review #8 fixes. Thanks mcasas@. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:51: gfx::Point squares[] = {{0, 0}, On 2016/10/25 22:13:12, mcasas wrote: > const ? > Also |start| in the next line. Done. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:58: for (unsigned i = 0; i < sizeof(squares) / sizeof(gfx::Point); ++i) { On 2016/10/25 22:13:12, mcasas wrote: > Suggestion: > > for (const auto& corner : squares) { > for (int y = corner.y(); y < corner.y() + side; ++y) { > for (int x = corner.x(); y < corner.x() + side; ++x) { > // same here > Done. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:61: unsigned value = On 2016/10/25 22:13:12, mcasas wrote: > const unsigned int? Done. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:146: } On 2016/10/25 22:13:12, mcasas wrote: > No need for {} in one-line bodies. Done. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:148: DrawGradientSquares(frame_format, data, elapsed_time, frame_size); On 2016/10/25 22:13:12, mcasas wrote: > So, just to make sure, I think you mentioned > elsewhere that this is not going to throw off > tests that expect some green pixels on pixels > in the upper left corner or its vicinity, correct? I run CQ in Patch Set 1: all passed - layout test issue on one of linux bots linux seems unrelated. webrtc tests like this one here [1] use central point: https://cs.chromium.org/chromium/src/content/browser/media/capture/web_conten... https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.cc:218: << "buffers"; On 2016/10/25 22:13:12, mcasas wrote: > nit: whitespace here before "buffers" and after "own" in l.217. Done. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:21: return media::PIXEL_FORMAT_I420; On 2016/10/25 22:13:12, mcasas wrote: > Suggestion: send the device index (|n| in l.49 > > media::VideoPixelFormat GetPixelFormat(int device_index) { > return device_index == 1 ? media::PIXEL_FORMAT_Y16 : media::PIXEL_FORMAT_I420; > } Changed the name but kept the parameter - the reason is usage in l.53 where I wanted to avoid string to int conversion. > > Note also the CamelCase naming for methods/functions. > This will also affect l.37. > > > A larger comment is that if #devices is 2, then we get an > I420 and an Y16; but with the current code, if we select > |device-count = 4| then we don't get two "couples" I420-Y16, > but more I420s. Is this intended/unintended/don't care ... ? Comment fixed - moved it to GetPixelFormat: // Factory has one device by default; I420. If there are more, the second device // is of Y16 format while the rest are I420. media::VideoPixelFormat GetPixelFormat(const std::string& device_id) { https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:135: unsigned count = 0; On 2016/10/25 22:13:12, mcasas wrote: > unsigned int? I think it's uncommon to see just |unsigned| > in Cr codebase. Done. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:140: static_cast<int>(number_of_devices_)); On 2016/10/25 22:13:12, mcasas wrote: > One line less if we do: > > number_of_devices_ = > std::min(kFakeCaptureMaxDeviceCount, > std::max(kFakeCaptureMinDeviceCount, static_cast<int>(count))); Done. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_unittest.cc:191: return client; On 2016/10/25 22:13:13, mcasas wrote: > Suggestion: > > return base::MakeUnique<MockClient>(base::Bind( > &FakeVideoCaptureDeviceBase::OnFrameCaptured, base::Unretained(this))); Done. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_unittest.cc:280: ASSERT_EQ(descriptors->size(), 3u); On 2016/10/25 22:13:12, mcasas wrote: > ASSERT_EQ(expected, actual), so here: > ASSERT_EQ(3u, descriptors->size()); > > I'm afraid the comparisons below are also reversed > (which is not your fault). > Done. I did it only in this method - will do the rest in follow up patch.
lgtm with some minor ideas. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:21: return media::PIXEL_FORMAT_I420; On 2016/10/25 23:20:40, aleksandar.stojiljkovic wrote: > On 2016/10/25 22:13:12, mcasas wrote: > > Suggestion: send the device index (|n| in l.49 > > > > media::VideoPixelFormat GetPixelFormat(int device_index) { > > return device_index == 1 ? media::PIXEL_FORMAT_Y16 : > media::PIXEL_FORMAT_I420; > > } > Changed the name but kept the parameter - the reason is usage in l.53 where I > wanted to avoid string to int conversion. I meant calling the function with the parameter |n| of the for loop in l.49, then you don't need to back-convert string to int, and the comparison in l.21 is trivial and can be done using the ternary operator in one line. > > > > > Note also the CamelCase naming for methods/functions. > > This will also affect l.37. > > > > > > A larger comment is that if #devices is 2, then we get an > > I420 and an Y16; but with the current code, if we select > > |device-count = 4| then we don't get two "couples" I420-Y16, > > but more I420s. Is this intended/unintended/don't care ... ? > > Comment fixed - moved it to GetPixelFormat: > // Factory has one device by default; I420. If there are more, the second device > // is of Y16 format while the rest are I420. > media::VideoPixelFormat GetPixelFormat(const std::string& device_id) { https://codereview.chromium.org/2447233002/diff/40001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2447233002/diff/40001/media/capture/video/fak... media/capture/video/fake_video_capture_device_factory.cc:102: command_line_parsed_ = true; nit; consider doing this flag management directly in l. 64. Also, s/parse_command_line/ParseCommandLine/ (I know it's not your fault but since you're in the area anyhow... )
Thanks for reviewing this. Patch Set 4 with nits fixes. https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:21: return media::PIXEL_FORMAT_I420; On 2016/10/26 02:03:15, mcasas wrote: > On 2016/10/25 23:20:40, aleksandar.stojiljkovic wrote: > > On 2016/10/25 22:13:12, mcasas wrote: > > > Suggestion: send the device index (|n| in l.49 > > > > > > media::VideoPixelFormat GetPixelFormat(int device_index) { > > > return device_index == 1 ? media::PIXEL_FORMAT_Y16 : > > media::PIXEL_FORMAT_I420; > > > } > > Changed the name but kept the parameter - the reason is usage in l.53 where I > > wanted to avoid string to int conversion. > > > I meant calling the function with the parameter |n| > of the for loop in l.49, then you don't need to back-convert > string to int, and the comparison in l.21 is trivial and can > be done using the ternary operator in one line. > > > > > > > > > Note also the CamelCase naming for methods/functions. > > > This will also affect l.37. > > > > > > > > > A larger comment is that if #devices is 2, then we get an > > > I420 and an Y16; but with the current code, if we select > > > |device-count = 4| then we don't get two "couples" I420-Y16, > > > but more I420s. Is this intended/unintended/don't care ... ? > > > > Comment fixed - moved it to GetPixelFormat: > > // Factory has one device by default; I420. If there are more, the second > device > > // is of Y16 format while the rest are I420. > > media::VideoPixelFormat GetPixelFormat(const std::string& device_id) { > Sorry, my previous explanation was wrong. l.53/l.94 - the reason is usage in l.94 where I wanted to avoid string to int conversion. Otherwise, ternary operator makes sense here so it is used in new patch. Previous code was this at because the plan is to add Y8 later (for. e.g. infrared 8 bit greyscale camera), but let's change that later. https://codereview.chromium.org/2447233002/diff/40001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2447233002/diff/40001/media/capture/video/fak... media/capture/video/fake_video_capture_device_factory.cc:102: command_line_parsed_ = true; On 2016/10/26 02:03:15, mcasas wrote: > nit; consider doing this flag management directly in l. 64. > > Also, s/parse_command_line/ParseCommandLine/ > (I know it's not your fault but since you're in the > area anyhow... ) Done, both good points.
On 2016/10/26 14:46:01, aleksandar.stojiljkovic wrote: > Thanks for reviewing this. > Patch Set 4 with nits fixes. > > https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... > File media/capture/video/fake_video_capture_device_factory.cc (right): > > https://codereview.chromium.org/2447233002/diff/1/media/capture/video/fake_vi... > media/capture/video/fake_video_capture_device_factory.cc:21: return > media::PIXEL_FORMAT_I420; > On 2016/10/26 02:03:15, mcasas wrote: > > On 2016/10/25 23:20:40, aleksandar.stojiljkovic wrote: > > > On 2016/10/25 22:13:12, mcasas wrote: > > > > Suggestion: send the device index (|n| in l.49 > > > > > > > > media::VideoPixelFormat GetPixelFormat(int device_index) { > > > > return device_index == 1 ? media::PIXEL_FORMAT_Y16 : > > > media::PIXEL_FORMAT_I420; > > > > } > > > Changed the name but kept the parameter - the reason is usage in l.53 where > I > > > wanted to avoid string to int conversion. > > > > > > I meant calling the function with the parameter |n| > > of the for loop in l.49, then you don't need to back-convert > > string to int, and the comparison in l.21 is trivial and can > > be done using the ternary operator in one line. > > > > > > > > > > > > > Note also the CamelCase naming for methods/functions. > > > > This will also affect l.37. > > > > > > > > > > > > A larger comment is that if #devices is 2, then we get an > > > > I420 and an Y16; but with the current code, if we select > > > > |device-count = 4| then we don't get two "couples" I420-Y16, > > > > but more I420s. Is this intended/unintended/don't care ... ? > > > > > > Comment fixed - moved it to GetPixelFormat: > > > // Factory has one device by default; I420. If there are more, the second > > device > > > // is of Y16 format while the rest are I420. > > > media::VideoPixelFormat GetPixelFormat(const std::string& device_id) { > > > > Sorry, my previous explanation was wrong. > l.53/l.94 - the reason is usage in l.94 where I wanted to avoid string to int > conversion. > Otherwise, ternary operator makes sense here so it is used in new patch. > Previous code was this at because the plan is to add Y8 later (for. e.g. > infrared 8 bit greyscale camera), but let's change that later. > > https://codereview.chromium.org/2447233002/diff/40001/media/capture/video/fak... > File media/capture/video/fake_video_capture_device_factory.cc (right): > > https://codereview.chromium.org/2447233002/diff/40001/media/capture/video/fak... > media/capture/video/fake_video_capture_device_factory.cc:102: > command_line_parsed_ = true; > On 2016/10/26 02:03:15, mcasas wrote: > > nit; consider doing this flag management directly in l. 64. > > > > Also, s/parse_command_line/ParseCommandLine/ > > (I know it's not your fault but since you're in the > > area anyhow... ) > > Done, both good points. Land this CL before https://crrev.com/2428263004/ and remove the Fake* changes from it, SG?
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
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/2447233002/#ps60001 (title: "Nits.")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
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/2447233002/#ps60001 (title: "Nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== FakeVideoCaptureDevice: Y16 testing support. Adds: - support for Y16 buffer format. - support for defining number of devices from command line. - generated color control points in corners that could be used to verify the frame rendering with no need for the reference render bitmap. BUG=624436 ========== to ========== FakeVideoCaptureDevice: Y16 testing support. Adds: - support for Y16 buffer format. - support for defining number of devices from command line. - generated color control points in corners that could be used to verify the frame rendering with no need for the reference render bitmap. BUG=624436 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== FakeVideoCaptureDevice: Y16 testing support. Adds: - support for Y16 buffer format. - support for defining number of devices from command line. - generated color control points in corners that could be used to verify the frame rendering with no need for the reference render bitmap. BUG=624436 ========== to ========== FakeVideoCaptureDevice: Y16 testing support. Adds: - support for Y16 buffer format. - support for defining number of devices from command line. - generated color control points in corners that could be used to verify the frame rendering with no need for the reference render bitmap. BUG=624436 Committed: https://crrev.com/38481bca66b6ca654bbe73cf6f0c418621d517cc Cr-Commit-Position: refs/heads/master@{#428001} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/38481bca66b6ca654bbe73cf6f0c418621d517cc Cr-Commit-Position: refs/heads/master@{#428001} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
