Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2735793002:
Add algorithm for processing constraints for video content capture. (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. |
DescriptionAdd algorithm for processing constraints for video content capture.
BUG=657733
Review-Url: https://codereview.chromium.org/2735793002
Cr-Commit-Position: refs/heads/master@{#457239}
Committed: https://chromium.googlesource.com/chromium/src/+/3e0109d24f32835337f58cfa0697eeb4d1cc2bc7
Patch Set 1 #Patch Set 2 : Fix to resolution-change policy #Patch Set 3 : Add contradiction tests #
Total comments: 14
Patch Set 4 : address hta's comments #
Total comments: 24
Patch Set 5 : Address review comments and rebase #Patch Set 6 : Add comment #Patch Set 7 : fix android/win_clang compilation issue #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 26 (13 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Adds algorithm for processing of constraints for video content capture. BUG=657733 ========== to ========== Adds algorithm for processing of constraints for video content capture. BUG=657733 ==========
guidou@chromium.org changed reviewers: + hbos@chromium.org, hta@chromium.org
Hi, PTAL. I still have to add a few more tests, but it is ready for review.
Description was changed from ========== Adds algorithm for processing of constraints for video content capture. BUG=657733 ========== to ========== Add algorithm for processing of constraints for video content capture. BUG=657733 ==========
Some worries. As a general matter, I don't like structs much - if we can have const classes, that's usually cleaner. https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.cc (right): https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:23: // Hard upper- and lower-bound frame rates for tab/desktop capture. Nit: Dashes shouldn't be here. "Hard upper and lower bounds for the frame rates for tab/desktop capture". https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:30: constexpr int kMaxScreenCastDimension = media::limits::kMaxDimension - 1; Still wonder why you do -1 on this number so that it's 32766 rather than 32767 (and if it matters). https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:124: !basic_constraint_set.aspectRatio.hasIdeal() && Here (and above) you're saying that if ideal is specified, the ability to adjust resolution is gone. This may be at odds with intention of "ideal", which is "you're free to vary, but prefer this". It may be the right thing to do for now, but the discrepancy (if there is one) needs mentioning in a comment. https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:183: std::string ideal_value = Should you loop over deviceId.ideal() here? for (const auto it : basic_constraint_set.deviceId.ideal()) { if (candidates.Contains(it->ascii()) return it->ascii(); } or something like that. {id: ideal: ["foo", "bar"]} is allowed by the WebIDL syntax. https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:232: result.failed_constraint_name = constraint_set.height.name(); Would it be as clear to have a constructor for result that took the failed constraint name as a parameter, and do return VideoContentCaptureSourceSelectionResult(constraint_set.height.name()) here? If doing that, can you avoid having the parameterless constructor, and make failed_constraint_name be an accessor to a private field? Is there anything else you really need to have changeable, as opposed to being able to pass it in as a constructor parameters? https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:243: result.failed_constraint_name = constraint_set.frameRate.name(); This choice seems odd - checking on device_id_iset and then returning "frameRate". https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.h (right): https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.h:41: Wonder if you could get away with a "private" here.
Description was changed from ========== Add algorithm for processing of constraints for video content capture. BUG=657733 ========== to ========== Add algorithm for processing constraints for video content capture. BUG=657733 ==========
PTAL. https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.cc (right): https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:23: // Hard upper- and lower-bound frame rates for tab/desktop capture. On 2017/03/08 13:34:08, hta - Chromium wrote: > Nit: Dashes shouldn't be here. "Hard upper and lower bounds for the frame rates > for tab/desktop capture". Done. https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:30: constexpr int kMaxScreenCastDimension = media::limits::kMaxDimension - 1; On 2017/03/08 13:34:08, hta - Chromium wrote: > Still wonder why you do -1 on this number so that it's 32766 rather than 32767 > (and if it matters). > The actual limit is kMaxDimension-1. See https://cs.chromium.org/chromium/src/media/capture/video_capture_types.cc?typ... For some reason, a format is valid only if its dimensions are less than (not <=) kMaxDimension. https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:124: !basic_constraint_set.aspectRatio.hasIdeal() && On 2017/03/08 13:34:08, hta - Chromium wrote: > Here (and above) you're saying that if ideal is specified, the ability to adjust > resolution is gone. > This may be at odds with intention of "ideal", which is "you're free to vary, > but prefer this". > > It may be the right thing to do for now, but the discrepancy (if there is one) > needs mentioning in a comment. After looking at what the policies actually do, using a new algorithm and added a separate test for this. Note that I decided to copy code from WebContentsCaptureMachine and added a TODO to make the code shareable. The copied code is simple enough that it was easier to copy it than to make an alternative "simple" implementation that required to be synced with WebContentsCaptureMachine anyway. WDYT? https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:183: std::string ideal_value = On 2017/03/08 13:34:08, hta - Chromium wrote: > Should you loop over deviceId.ideal() here? > > for (const auto it : basic_constraint_set.deviceId.ideal()) { > if (candidates.Contains(it->ascii()) > return it->ascii(); > } > > or something like that. > {id: ideal: ["foo", "bar"]} is allowed by the WebIDL syntax. Done. Also added test for this. https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:232: result.failed_constraint_name = constraint_set.height.name(); On 2017/03/08 13:34:08, hta - Chromium wrote: > Would it be as clear to have a constructor for result that took the failed > constraint name as a parameter, and do > > return VideoContentCaptureSourceSelectionResult(constraint_set.height.name()) > > here? > If doing that, can you avoid having the parameterless constructor, and make > failed_constraint_name be an accessor to a private field? > > Is there anything else you really need to have changeable, as opposed to being > able to pass it in as a constructor parameters? Done. https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:243: result.failed_constraint_name = constraint_set.frameRate.name(); On 2017/03/08 13:34:08, hta - Chromium wrote: > This choice seems odd - checking on device_id_iset and then returning > "frameRate". Fixed. In practice this can't be reached because any string is currently accepted as device_id, as there is no way to have an enumeration of valid device IDs here. This is the reason it wasn't caught by an Overconstrained test (there is no way to overconstrain the ID at this stage). If an invalid device ID is given, that is handled at a later stage. https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.h (right): https://codereview.chromium.org/2735793002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.h:41: On 2017/03/08 13:34:09, hta - Chromium wrote: > Wonder if you could get away with a "private" here. Done. Changed to a class.
https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.cc (right): https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:25: constexpr double kMinScreenCastFrameRate = 1.0 / 60.0; This is what I call moving pictures! :) https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:38: using StringSet = DiscreteSet<std::string>; Move all using statements to before functions are being defined. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:104: media::ResolutionChangePolicy SelectResolutionPolicyFromCandidates( Can you add a comment? I'm not sure I understand the purpose of this function or what the resulting policy means. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:116: return percentage_diff <= 1; // Effectively, anything strictly <2%. Why <2%? Why not exactly (within numerical tolerance), or if not exactly why such a small percentage and not a larger tolerance? https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:131: adjusted_size = RoundToExactAspectRatio(64, 48); Why these step sizes and not the smallest possible integer steps? 16x9 and 4x3? https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:186: DCHECK(params.IsValid()); Note: Will always use default PowerLineFrequency. That is on purpose? https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:205: return std::string(); Should this return Optional<std::string> instead? https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:208: return candidates.FirstElement(); Is the preference for the first device id intentional or arbitrarily "any will do"? Add comment. Similar question for other return FirstElement places. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:238: std::move(device_id), noise_reduction, capture_params); What if device_id is the empty string? Shouldn't that be failed constraint deviceId? Otherwise document the meaning of empty string. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.h (right): https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.h:26: // Creates a result with the given values. Takes ownership of |device_id|. Since |device_id| is passed by value it is essentially a local variable, owned by the function by definition. I think the "takes ownership" comment is redundant and confusing. Remove?
lgtm One comment about documentation of ownership needed, otherwise let's get the show on the road. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.h (right): https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.h:24: const char* failed_constraint_name); In this case, the ownership of failed_constraint_name is unclear (since it's a char*, and you store the pointer rather than a copy of the pointed-to string). The destructor of this class is "default", so it seems it does not attempt to release the failed_constraint_name pointer, so it doesn't take ownership, and the caller must ensure that it's a string that will remain accessible. Please document. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.h:26: // Creates a result with the given values. Takes ownership of |device_id|. On 2017/03/09 16:51:10, hbos_chromium wrote: > Since |device_id| is passed by value it is essentially a local variable, owned > by the function by definition. I think the "takes ownership" comment is > redundant and confusing. Remove? It should probably be const std::string& since it's copied into a local std::string.
Hi, PTAL https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.cc (right): https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:25: constexpr double kMinScreenCastFrameRate = 1.0 / 60.0; On 2017/03/09 16:51:10, hbos_chromium wrote: > This is what I call moving pictures! :) Acknowledged. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:38: using StringSet = DiscreteSet<std::string>; On 2017/03/09 16:51:09, hbos_chromium wrote: > Move all using statements to before functions are being defined. Done. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:104: media::ResolutionChangePolicy SelectResolutionPolicyFromCandidates( On 2017/03/09 16:51:09, hbos_chromium wrote: > Can you add a comment? I'm not sure I understand the purpose of this function or > what the resulting policy means. Changed the policy to be simpler and added a TODO for what I tried in this patchset. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:116: return percentage_diff <= 1; // Effectively, anything strictly <2%. On 2017/03/09 16:51:09, hbos_chromium wrote: > Why <2%? Why not exactly (within numerical tolerance), or if not exactly why > such a small percentage and not a larger tolerance? See previous comment. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:131: adjusted_size = RoundToExactAspectRatio(64, 48); On 2017/03/09 16:51:09, hbos_chromium wrote: > Why these step sizes and not the smallest possible integer steps? 16x9 and 4x3? See previous comment. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:186: DCHECK(params.IsValid()); On 2017/03/09 16:51:10, hbos_chromium wrote: > Note: Will always use default PowerLineFrequency. That is on purpose? Yes. PowerLineFrequency is ignored for content capture. Added comment. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:205: return std::string(); On 2017/03/09 16:51:10, hbos_chromium wrote: > Should this return Optional<std::string> instead? No. The default value for device_id is the empty string. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:208: return candidates.FirstElement(); On 2017/03/09 16:51:09, hbos_chromium wrote: > Is the preference for the first device id intentional or arbitrarily "any will > do"? Add comment. > Similar question for other return FirstElement places. Added comment here and in the other call to FirstElement. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:238: std::move(device_id), noise_reduction, capture_params); On 2017/03/09 16:51:09, hbos_chromium wrote: > What if device_id is the empty string? Shouldn't that be failed constraint > deviceId? Otherwise document the meaning of empty string. Empty string is treated like any other ID, just like in the old algorithm. I don't think special documentation for it is needed in this particular part of the code, but added a comment in SelectDeviceIDFromCandidates that says the empty string is treated as a default value for device ID. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.h (right): https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.h:24: const char* failed_constraint_name); On 2017/03/10 08:51:46, hta - Chromium wrote: > In this case, the ownership of failed_constraint_name is unclear (since it's a > char*, and you store the pointer rather than a copy of the pointed-to string). > The destructor of this class is "default", so it seems it does not attempt to > release the failed_constraint_name pointer, so it doesn't take ownership, and > the caller must ensure that it's a string that will remain accessible. Please > document. > Done. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.h:26: // Creates a result with the given values. Takes ownership of |device_id|. On 2017/03/09 16:51:10, hbos_chromium wrote: > Since |device_id| is passed by value it is essentially a local variable, owned > by the function by definition. I think the "takes ownership" comment is > redundant and confusing. Remove? I've seen this wording used for when the values is move. Changed to "moved to an internal field", which is perhaps clearer. https://codereview.chromium.org/2735793002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.h:26: // Creates a result with the given values. Takes ownership of |device_id|. On 2017/03/10 08:51:46, hta - Chromium wrote: > On 2017/03/09 16:51:10, hbos_chromium wrote: > > Since |device_id| is passed by value it is essentially a local variable, owned > > by the function by definition. I think the "takes ownership" comment is > > redundant and confusing. Remove? > > It should probably be const std::string& since it's copied into a local > std::string. It is actually moved (not copied) to a local std::string. That's the reason it's passed by value (and that the style guide forbids using r-value references). Updated comment to indicate that more explicitly.
lgtm
guidou@chromium.org changed reviewers: + avi@chromium.org
avi@: Can you rs?
lgtm stampity stamp
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, hbos@chromium.org Link to the patchset: https://codereview.chromium.org/2735793002/#ps120001 (title: "Add comment")
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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, hta@chromium.org, hbos@chromium.org Link to the patchset: https://codereview.chromium.org/2735793002/#ps140001 (title: "fix android compilation issue")
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": 140001, "attempt_start_ts": 1489612190424180,
"parent_rev": "e3d8f58563aececeeaa395795e022ddadf23c93d", "commit_rev":
"3e0109d24f32835337f58cfa0697eeb4d1cc2bc7"}
Message was sent while issue was closed.
Description was changed from ========== Add algorithm for processing constraints for video content capture. BUG=657733 ========== to ========== Add algorithm for processing constraints for video content capture. BUG=657733 Review-Url: https://codereview.chromium.org/2735793002 Cr-Commit-Position: refs/heads/master@{#457239} Committed: https://chromium.googlesource.com/chromium/src/+/3e0109d24f32835337f58cfa0697... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/3e0109d24f32835337f58cfa0697... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
