Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2719153002:
Patch constraints algorithm for device video-capture sources. (Closed)
|
Created:
3 years, 9 months ago by Guido Urdaneta Modified:
3 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, imcheng+watch_chromium.org, feature-media-reviews_chromium.org, posciak+watch_chromium.org, chfremer+watch_chromium.org, jam, avayvod+watch_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, jasonroberts+watch_google.com, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPatch constraints algorithm for device video-capture sources.
The original algorithm did not correctly handle cases of contradicting
advanced constraint sets for source settings that support a range of values
(i.e., height, width and frame rate).
This patch fixes this glitch, fixes two existing tests and adds new
tests.
Thanks hbos@ for noticing this glitch in a previous review.
BUG=657733
some good progress
Review-Url: https://codereview.chromium.org/2719153002
Cr-Commit-Position: refs/heads/master@{#453941}
Committed: https://chromium.googlesource.com/chromium/src/+/1c4341272631c917395e82ff2cea99098b9eaded
Patch Set 1 #Patch Set 2 : add extra tests for string and powerline frequency for improved coverage #
Total comments: 9
Patch Set 3 : address comments by hbos #
Total comments: 4
Patch Set 4 : improve comments based on hta's suggestions, fix bug and modify test to catch that bug. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 24 (15 generated)
guidou@chromium.org changed reviewers: + hbos@chromium.org, hta@chromium.org
Hi, PTAL
The CQ bit was checked by guidou@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: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by guidou@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_device.cc (right): https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device.cc:110: // that support a range of values (height, width and frame rate). I think it is not clear what this is doing considering we brute-force iterate each format. We look at each case, e.g. a resolution, so how does the ranges come into play? A resolution either is or is not supported by a given constraint, one might think. Can you clarify this comment by adding an example, e.g. camera: w = 1024 adv1: w <= 640 adv2: w >= 800 Here, the camera width can be cropped to support adv1 (with a penalty for cropping), or it could satisfy adv2 because 1024 >= 800. On a set-by-set basis the algorithm would say both are satisfied, but it can't do both at the same time. Because constraint sets are applied in order, adv2 should be rejected (INF) when adv1 is applied. As such this class is needed to keep track of the so-far constrained format (range) when iterating advanced sets. https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device.cc:652: // FormatSourceDistance(candidate, constraint_set, nullptr))); Why is this commented-out? https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_device_unittest.cc (right): https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device_unittest.cc:1247: TEST_F(MediaStreamConstraintsUtilVideoDeviceTest, AdvancedExactResolution) { This name no longer matches what the test is doing. https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device_unittest.cc:1259: // Tie-breaker rule that applies is closeness to default settings. adv1 is superflous given adv2. What about...? adv1: >= 4000 - not supported by any format SelectSettings() and make sure we get default adv2: <= 3000 - supportable but contradicting adv1, which is OK since adv1 cannot be supported anyway. SelectSettings() and make sure we still get default adv3: >= 1920 - supportable and not contradicting adv2. SelectSettings() and make sure we get HD I wonder if this can be improved even further by having adv2 affect which resolution is chosen (non-default) and then having adv3 make it chose yet another resolution so we know all sets are in play. With the current example it could be the case that adv2 is ignored because all formats apply to it. But maybe that would break what is happening now with adv3 changing the resolution. I think going from default to HD is OK too. We don't want adv3 to contradict adv2. https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device_unittest.cc:1424: EXPECT_EQ(480, result.Height()); These tests are all good, but we want if possible test coverage of what the ConstrainedFormat is doing. This test would pass even without that class, because a format that can satisfy the first but not the second (640x480) is already better than a format that can satisfy the second but not the first (1920x1080). We need something like I mentioned in the earlier comment with camera: w = 1024 adv1: w <= 640 adv2: w >= 800 Can we distinguish between adv2 being rejected and not being rejected? What if we have: camera1: 1024@50Hz camera2: 1024@60Hz adv1: w <= 640 adv2: w >= 800, 60Hz adv3: 50Hz Without ConstrainedSet it should choose camera2 because it would falsely think it could satisfy both adv1 and adv2. With ConstrainedSet it should choose camera1 because adv2 is unsatisfiable but adv1+adv3 isn't. We can distinguish between adv3 being used or not if 50Hz is not the default Hz. The test could do one SelectSetting with adv1+adv2 and get default Hz and one with adv1+adv2+adv3 and get adv3's Hz. One test like that for each value range maybe?
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_device.cc (right): https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device.cc:110: // that support a range of values (height, width and frame rate). On 2017/02/28 11:41:53, hbos_chromium wrote: > I think it is not clear what this is doing considering we brute-force iterate > each format. We look at each case, e.g. a resolution, so how does the ranges > come into play? A resolution either is or is not supported by a given > constraint, one might think. Can you clarify this comment by adding an example, > > e.g. camera: w = 1024 > adv1: w <= 640 > adv2: w >= 800 > > Here, the camera width can be cropped to support adv1 (with a penalty for > cropping), or it could satisfy adv2 because 1024 >= 800. On a set-by-set basis > the algorithm would say both are satisfied, but it can't do both at the same > time. Because constraint sets are applied in order, adv2 should be rejected > (INF) when adv1 is applied. As such this class is needed to keep track of the > so-far constrained format (range) when iterating advanced sets. Done. https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device.cc:652: // FormatSourceDistance(candidate, constraint_set, nullptr))); On 2017/02/28 11:41:53, hbos_chromium wrote: > Why is this commented-out? Removed. |candidate| is no longer available here. https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_device_unittest.cc (right): https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device_unittest.cc:1259: // Tie-breaker rule that applies is closeness to default settings. On 2017/02/28 11:41:54, hbos_chromium wrote: > adv1 is superflous given adv2. > > What about...? > adv1: >= 4000 - not supported by any format > SelectSettings() and make sure we get default > > adv2: <= 3000 - supportable but contradicting adv1, > which is OK since adv1 cannot be supported anyway. > SelectSettings() and make sure we still get default > > adv3: >= 1920 - supportable and not contradicting adv2. > SelectSettings() and make sure we get HD > > I wonder if this can be improved even further by having adv2 affect which > resolution is chosen (non-default) and then having adv3 make it chose yet > another resolution so we know all sets are in play. With the current example it > could be the case that adv2 is ignored because all formats apply to it. But > maybe that would break what is happening now with adv3 changing the resolution. > I think going from default to HD is OK too. We don't want adv3 to contradict > adv2. Replaced with a test where each advanced set causes a device change. https://codereview.chromium.org/2719153002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device_unittest.cc:1424: EXPECT_EQ(480, result.Height()); On 2017/02/28 11:41:54, hbos_chromium wrote: > These tests are all good, but we want if possible test coverage of what the > ConstrainedFormat is doing. > > This test would pass even without that class, because a format that can satisfy > the first but not the second (640x480) is already better than a format that can > satisfy the second but not the first (1920x1080). > > We need something like I mentioned in the earlier comment with > camera: w = 1024 > adv1: w <= 640 > adv2: w >= 800 > > Can we distinguish between adv2 being rejected and not being rejected? > > What if we have: > camera1: 1024@50Hz > camera2: 1024@60Hz > adv1: w <= 640 > adv2: w >= 800, 60Hz > adv3: 50Hz > > Without ConstrainedSet it should choose camera2 because it would falsely think > it could satisfy both adv1 and adv2. > With ConstrainedSet it should choose camera1 because adv2 is unsatisfiable but > adv1+adv3 isn't. We can distinguish between adv3 being used or not if 50Hz is > not the default Hz. > > The test could do one SelectSetting with adv1+adv2 and get default Hz and one > with adv1+adv2+adv3 and get adv3's Hz. > > One test like that for each value range maybe? Added tests similar to the proposed ones with width/frameRate and height/frameRate.
lgtm Some suggestions on how to explain things better, including one which I think is important (what effect |constrained_format| can have) because I think it's important for correctness, and hard to detect by reading the source. https://codereview.chromium.org/2719153002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_device.cc (right): https://codereview.chromium.org/2719153002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device.cc:617: // |candidate| and |constrained_format|. The returned value is the sum of the I think a better formulation for the first sentence would be 2 sentences: Returns the fitness distance between |constraint_set| and |candidate| given that the configuration is already constrained by |constrained_format|. |constrained_format| may cause the distance to be infinite if the new constraint can't be satisfied together with the previous ones, but will not influence the numeric value returned if it is not infinite. (I think that's a true statement. I'm not 100% sure it is.) https://codereview.chromium.org/2719153002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_device_unittest.cc (right): https://codereview.chromium.org/2719153002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device_unittest.cc:1254: // No device supports the first advanced set. Add a comment: This first advanced constraint is therefore ignored in all calls to SelectSettings(). When reading this test, it is important to notice that the new constraints are added to the end of the list of already existing constraints (the factory is not reset between calls). The naming emphasizes that, which is good.
Hi, PTAL.
https://codereview.chromium.org/2719153002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_device.cc (right): https://codereview.chromium.org/2719153002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device.cc:617: // |candidate| and |constrained_format|. The returned value is the sum of the On 2017/02/28 19:32:21, hta - Chromium wrote: > I think a better formulation for the first sentence would be 2 sentences: > > Returns the fitness distance between |constraint_set| and |candidate| given that > the configuration is already constrained by |constrained_format|. > > |constrained_format| may cause the distance to be infinite if the new constraint > can't be satisfied together with the previous ones, but will not influence the > numeric value returned if it is not infinite. > > (I think that's a true statement. I'm not 100% sure it is.) Done. FitnessDistance cannot be infinite because it is called only after the basic constraint set is known to be satisfied (see DCHECK), but the comment is valid to the various SourceDistance functions, whose comments were rewritten to include the suggested wording. https://codereview.chromium.org/2719153002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_device_unittest.cc (right): https://codereview.chromium.org/2719153002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_device_unittest.cc:1254: // No device supports the first advanced set. On 2017/02/28 19:32:21, hta - Chromium wrote: > Add a comment: This first advanced constraint is therefore ignored in all calls > to SelectSettings(). > > When reading this test, it is important to notice that the new constraints are > added to the end of the list of already existing constraints (the factory is not > reset between calls). The naming emphasizes that, which is good. Done.
lgtm
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@chromium.org Link to the patchset: https://codereview.chromium.org/2719153002/#ps100001 (title: "improve comments based on hta's suggestions, fix bug and modify test to catch that bug.")
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": 1488380808689550,
"parent_rev": "e1ac111de29d1ca2f661c5ff46a6897495f389d5", "commit_rev":
"1c4341272631c917395e82ff2cea99098b9eaded"}
Message was sent while issue was closed.
Description was changed from ========== Patch constraints algorithm for device video-capture sources. The original algorithm did not correctly handle cases of contradicting advanced constraint sets for source settings that support a range of values (i.e., height, width and frame rate). This patch fixes this glitch, fixes two existing tests and adds new tests. Thanks hbos@ for noticing this glitch in a previous review. BUG=657733 some good progress ========== to ========== Patch constraints algorithm for device video-capture sources. The original algorithm did not correctly handle cases of contradicting advanced constraint sets for source settings that support a range of values (i.e., height, width and frame rate). This patch fixes this glitch, fixes two existing tests and adds new tests. Thanks hbos@ for noticing this glitch in a previous review. BUG=657733 some good progress Review-Url: https://codereview.chromium.org/2719153002 Cr-Commit-Position: refs/heads/master@{#453941} Committed: https://chromium.googlesource.com/chromium/src/+/1c4341272631c917395e82ff2cea... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/1c4341272631c917395e82ff2cea... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
