Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1135823004:
Implement all resolution change policies for desktop and tab capture. (Closed)
|
Created:
5 years, 7 months ago by miu Modified:
5 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org, jam, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@resolution_change_policy_constraints_ITEM1_CR1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement all resolution change policies for desktop and tab capture.
Prior to this change, desktop/window capture would emit frames of any
size, regardless of the resolution change policy; and tab capture would
always emit frames of a fixed resolution. Both cause a problem where
some receivers of the captured video need to do scaling/letterboxing
themselves for proper UX, and other receivers require this be done on
by the capture pipeline.
This change builds upon https://codereview.chromium.org/1124263004,
which allows the client to specify capture constraints that configure
the capture pipeline to do (or not do) the scaling/letterboxing. All
frame resolution calculation logic has been factored into one place in a
new CaptureResolutionChooser class, which also depends on new utility
functions added to media/base/video_util.*.
BUG=473336
Committed: https://crrev.com/39254e67228717bbddf222614abd6cb9ba38608c
Cr-Commit-Position: refs/heads/master@{#330232}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed Dale's comments. #
Total comments: 101
Patch Set 3 : Addressed Wez's first round comments. #Patch Set 4 : REBASE #Patch Set 5 : Addressed Wez's second round comments. #
Total comments: 8
Patch Set 6 : Addressed Wez's third round comments. #
Total comments: 2
Patch Set 7 : !!rwh to rwh != nullptr #Patch Set 8 : REBASE #Messages
Total messages: 18 (3 generated)
miu@chromium.org changed reviewers: + dalecurtis@chromium.org, wez@chromium.org
dalecurtis: PTAL at media/base/video_util* wez: PTAL at content/browser/media/capture/*
media/base/video_util lgtm % static func instead of templating. https://codereview.chromium.org/1135823004/diff/1/media/base/video_util.cc File media/base/video_util.cc (right): https://codereview.chromium.org/1135823004/diff/1/media/base/video_util.cc#ne... media/base/video_util.cc:276: template<bool fit_within_target> Seems like this is just a static function with an extra bool parameter? https://codereview.chromium.org/1135823004/diff/1/media/base/video_util.cc#ne... media/base/video_util.cc:322: else Remove else?
https://codereview.chromium.org/1135823004/diff/1/media/base/video_util.cc File media/base/video_util.cc (right): https://codereview.chromium.org/1135823004/diff/1/media/base/video_util.cc#ne... media/base/video_util.cc:276: template<bool fit_within_target> On 2015/05/09 01:50:21, DaleCurtis wrote: > Seems like this is just a static function with an extra bool parameter? Done. https://codereview.chromium.org/1135823004/diff/1/media/base/video_util.cc#ne... media/base/video_util.cc:322: else On 2015/05/09 01:50:21, DaleCurtis wrote: > Remove else? Done.
Haven't quite finished reviewing video capture device changes, but here's initial comments. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:36: break; nit: Suggest returning Size(1,1) explicitly here, for clarity. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:37: case media::RESOLUTION_POLICY_LAST: Won't this raise compilation errors since you're handling both LAST and whatever the other name for the last value is? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:40: return gfx::Size(1, 1); nit: Can you get rid of this if you add a return to the LAST case? Then all your cases have clear, explicit results. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:44: // the bounds are exceeded, a uniformly scaled |size| is returned that is within nit: Is "uniformly scaled" sufficiently well-understood to be clear here? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:113: void CaptureResolutionEvaluator::UpdateForNewCapabilityLimit(int num_pixels) { Is the rationale for including this in this CL rather than in the follow up CL just that the calling code wants to call it even though it's not yet implemented? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:119: // capture resolution, given the current capabilities of the system. Bug # for that change? :) https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator.h (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:19: class CONTENT_EXPORT CaptureResolutionEvaluator { nit: Evaluator doesn't seem the right name for this; CaptureResolutionChooser? Or ..Selector or ..Calculator? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:23: media::ResolutionChangePolicy resolution_change_policy); Suggest briefly explaining what resolution_change_policy can be. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:33: void UpdateForNewSourceSize(const gfx::Size& source_size); nit: I'd suggest naming these SetSourceSize() and SetMaxPixelsPerFrame() https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:36: // the system, in terms of |num_pixels| per frame. I think it'd be clearer to split what the function does from _why_ it does it, e.g: "Updates the capture size to not exceed |num_pixels| per frame. This is useful in limiting capture resource consumption (CPU, memory bandwidth), which is typically proportional to pixels-per-frame." https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:41: void UpdateCaptureSize(); If you update the others to Set... then suggest this become RefreshCaptureSize(), or RecalculateCaptureSize(). https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:43: // Hard constraints. Where does |min_frame_size_| come from - it doesn't seem to be a parameter? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:47: // Selects the set of heuristics to use. nit: Selects -> Specifies https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:51: // the capability metric. This isn't very clear; naively I'd assume this must be the same as the source size but I think it takes into account the resolution policy and max/min constraints, but not the pixel per frame limit? In which case consider calling this constrained_size_? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:55: gfx::Size capture_size_; nit: "The current computed capture frame resolution." https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:12: nit: Don't think you need an empty namespace since const into will implicitly have static linkage, below? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:22: FixedResolutionPolicy_CaptureSizeAlwaysFixed) { nit: Suggest putting a blank line between each call+expectation pair, so this reads more clearly as a sequence of updates & checks. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:46: #define EXPECT_IS_WITHIN_BOUNDS_AND_SAME_ASPECT_RATIO() \ Might be cleaner to take the hit of parameterizing this on a Size and making it global to this code file, so you don't need to undefined it, below? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:70: const gfx::Size unchanged_size = evaluator.capture_size(); Looks like this belongs in the next block? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:125: const gfx::Size unchanged_size = evaluator.capture_size(); As above https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:154: if (task_runner_->BelongsToCurrentThread()) { So the task runner is sometimes UI thread and sometimes not...? https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.cc File media/base/video_util.cc (right): https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.c... media/base/video_util.cc:282: // Scale |size|, preserving aspect ratio. nit: This comment adds nothing, since that's the point of the function. ;) https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h File media/base/video_util.h (right): https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h... media/base/video_util.h:89: // is preserved as closely as possible. If |size| is empty, its aspect ratio nit: Aspect ratio of |size| is preserved as closely as possible. https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h... media/base/video_util.h:96: // is preserved as closely as possible. If |size| is empty, its aspect ratio nit: You mean the aspect ratio of |size| is preserved. https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h... media/base/video_util.h:97: // would be undefined; and in this case an empty Size would be returned. I'd suggest just saying that the result is always empty if |size| is empty, here and above. https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h... media/base/video_util.h:102: // matches the aspect ratio of |target|. If |target| is empty, its aspect ratio It's not immediately obvious how this differs from ScaleSizeToEncompassTarget() - is the difference that this function won't down-size if |size| is already larger than |target| in both dimensions? https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h... media/base/video_util.h:103: // would be undefined; and in this case an empty Size would be returned. Surely the same is true if |size| is empty? Suggest saying that if either of |size| or |target| are empty then the result will be empty. nit: Here and above, would it make more sense to CHECK() that these parameters are not empty, since empty typically means that callers are doing the wrong thing?
Thanks for the initial comments. Addressed all of them. PTAL. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:36: break; On 2015/05/13 19:05:08, Wez wrote: > nit: Suggest returning Size(1,1) explicitly here, for clarity. Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:37: case media::RESOLUTION_POLICY_LAST: On 2015/05/13 19:05:08, Wez wrote: > Won't this raise compilation errors since you're handling both LAST and whatever > the other name for the last value is? This is the "meaningless" enum value that only exists for the IPC layer to check for a valid enum range. In other words, this switch statement has a case for exactly one of each possible enum value. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:40: return gfx::Size(1, 1); On 2015/05/13 19:05:08, Wez wrote: > nit: Can you get rid of this if you add a return to the LAST case? Then all your > cases have clear, explicit results. MSVC++ compiler complains, unfortunately, even though it's obvious the code point couldn't be reached. Unless you're aware they've fixed this problem recently? I'll give it a shot and see what happens on the trybots. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:44: // the bounds are exceeded, a uniformly scaled |size| is returned that is within On 2015/05/13 19:05:08, Wez wrote: > nit: Is "uniformly scaled" sufficiently well-understood to be clear here? Done. Clarified. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:113: void CaptureResolutionEvaluator::UpdateForNewCapabilityLimit(int num_pixels) { On 2015/05/13 19:05:08, Wez wrote: > Is the rationale for including this in this CL rather than in the follow up CL > just that the calling code wants to call it even though it's not yet > implemented? No. I decided to add it in as a skeleton method so that it's clear in this change why I'm going to all the trouble of adding a whole separate class to encapsulate the resolution computation logic. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:119: // capture resolution, given the current capabilities of the system. On 2015/05/13 19:05:08, Wez wrote: > Bug # for that change? :) Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator.h (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:19: class CONTENT_EXPORT CaptureResolutionEvaluator { On 2015/05/13 19:05:08, Wez wrote: > nit: Evaluator doesn't seem the right name for this; CaptureResolutionChooser? > Or ..Selector or ..Calculator? Done. I like "chooser," since an upcoming change will introduce logic to "snap to" one of a specific set of resolutions. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:23: media::ResolutionChangePolicy resolution_change_policy); On 2015/05/13 19:05:09, Wez wrote: > Suggest briefly explaining what resolution_change_policy can be. Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:33: void UpdateForNewSourceSize(const gfx::Size& source_size); On 2015/05/13 19:05:08, Wez wrote: > nit: I'd suggest naming these SetSourceSize() and SetMaxPixelsPerFrame() Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:36: // the system, in terms of |num_pixels| per frame. On 2015/05/13 19:05:09, Wez wrote: > I think it'd be clearer to split what the function does from _why_ it does it, > e.g: > > "Updates the capture size to not exceed |num_pixels| per frame. This is useful > in limiting capture resource consumption (CPU, memory bandwidth), which is > typically proportional to pixels-per-frame." Done. I liked your wording, so I plagiarized it. ;-) https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:41: void UpdateCaptureSize(); On 2015/05/13 19:05:09, Wez wrote: > If you update the others to Set... then suggest this become > RefreshCaptureSize(), or RecalculateCaptureSize(). Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:43: // Hard constraints. On 2015/05/13 19:05:08, Wez wrote: > Where does |min_frame_size_| come from - it doesn't seem to be a parameter? Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:47: // Selects the set of heuristics to use. On 2015/05/13 19:05:08, Wez wrote: > nit: Selects -> Specifies Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:51: // the capability metric. On 2015/05/13 19:05:09, Wez wrote: > This isn't very clear; naively I'd assume this must be the same as the source > size but I think it takes into account the resolution policy and max/min > constraints, but not the pixel per frame limit? > > In which case consider calling this constrained_size_? Done. Agreed, I like your naming better. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.h:55: gfx::Size capture_size_; On 2015/05/13 19:05:08, Wez wrote: > nit: "The current computed capture frame resolution." Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:12: On 2015/05/13 19:05:09, Wez wrote: > nit: Don't think you need an empty namespace since const into will implicitly > have static linkage, below? I don't want to export these symbols. I could remove the anonymous namespace and add the static keyword, but that's just a matter of taste. ;-) https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:22: FixedResolutionPolicy_CaptureSizeAlwaysFixed) { On 2015/05/13 19:05:09, Wez wrote: > nit: Suggest putting a blank line between each call+expectation pair, so this > reads more clearly as a sequence of updates & checks. Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:46: #define EXPECT_IS_WITHIN_BOUNDS_AND_SAME_ASPECT_RATIO() \ On 2015/05/13 19:05:09, Wez wrote: > Might be cleaner to take the hit of parameterizing this on a Size and making it > global to this code file, so you don't need to undefined it, below? Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:70: const gfx::Size unchanged_size = evaluator.capture_size(); On 2015/05/13 19:05:09, Wez wrote: > Looks like this belongs in the next block? Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:125: const gfx::Size unchanged_size = evaluator.capture_size(); On 2015/05/13 19:05:09, Wez wrote: > As above Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:154: if (task_runner_->BelongsToCurrentThread()) { On 2015/05/13 19:05:09, Wez wrote: > So the task runner is sometimes UI thread and sometimes not...? tl;dr: This is the pattern decided upon, based on previous design/impl decisions. WebContentsTracker is shared by both WebContentsVideoCaptureDevice and WebContentsAudioInputStream. The former uses this class exclusively on the UI thread, while the latter uses it from a separate media thread. The "tracking" and "observing" logic is the same for both. While, granted, the WCAudioInputStream won't ever use this new "resize observing" logic, I wanted to remain consistent with the threading model of the rest of this class. https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.cc File media/base/video_util.cc (right): https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.c... media/base/video_util.cc:282: // Scale |size|, preserving aspect ratio. On 2015/05/13 19:05:09, Wez wrote: > nit: This comment adds nothing, since that's the point of the function. ;) Done. https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h File media/base/video_util.h (right): https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h... media/base/video_util.h:89: // is preserved as closely as possible. If |size| is empty, its aspect ratio On 2015/05/13 19:05:09, Wez wrote: > nit: Aspect ratio of |size| is preserved as closely as possible. Done. https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h... media/base/video_util.h:96: // is preserved as closely as possible. If |size| is empty, its aspect ratio On 2015/05/13 19:05:09, Wez wrote: > nit: You mean the aspect ratio of |size| is preserved. Done. https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h... media/base/video_util.h:97: // would be undefined; and in this case an empty Size would be returned. On 2015/05/13 19:05:09, Wez wrote: > I'd suggest just saying that the result is always empty if |size| is empty, here > and above. Done. https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h... media/base/video_util.h:102: // matches the aspect ratio of |target|. If |target| is empty, its aspect ratio On 2015/05/13 19:05:09, Wez wrote: > It's not immediately obvious how this differs from ScaleSizeToEncompassTarget() > - is the difference that this function won't down-size if |size| is already > larger than |target| in both dimensions? Done. Elaborated further in the comment. https://codereview.chromium.org/1135823004/diff/20001/media/base/video_util.h... media/base/video_util.h:103: // would be undefined; and in this case an empty Size would be returned. On 2015/05/13 19:05:09, Wez wrote: > Surely the same is true if |size| is empty? Yes. > Suggest saying that if either of |size| or |target| are empty then the result > will be empty. Done. > nit: Here and above, would it make more sense to CHECK() that these parameters > are not empty, since empty typically means that callers are doing the wrong > thing? Perhaps. A quick look-up shows there are 14+ callers of ComputeLetterboxRegion(). I'm not sure if there are assumptions being made that an empty input is valid and should produce an empty result. I took a look at the new functions, and the default behavior is actually reasonable in some cases. For example, when we call ScaleToEncompassTarget() with an empty target, it makes logical sense that the result should also be an empty Size. As these are shared media utility functions, I'll discuss this with Dale, and follow-up in a later change if the CHECK() strategy is more appropriate.
Moar comments! https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/content_video_capture_device_core.cc (left): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/content_video_capture_device_core.cc:248: // will convert to) YUV420. Do we need to worry about the I420 even-size limitations here? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:357: // changed at least twice while capturing. nit: Why is this not a loop? Why do we care that the source frame size has changed at least twice, versus just once? If the requirement is that it has changed at least twice, could we simply compare received frame sizes between OnIncomingCapturedData() invocations? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:367: for (const media::VideoCaptureFormat& f : formats) { nit: Could this be: for (auto format : formats) ? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:377: EXPECT_EQ(2u, distinct_sizes_seen.size()); Is there something about the mock capturer that causes it to output two specific sets of dimensions? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:378: for (const gfx::Size& s : distinct_sizes_seen) nit: auto here? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:441: EXPECT_FALSE(have_seen_other_frame_size); nit: The fixed aspect-ratio test collates a list of the sizes seen and checks e.g. that there are exactly two of them - you could do the same here, plus test that the two test frame sizes are contained in the list, and the resulting code looks like it'd be more concise? Either way, it seems that using the same basic approach between the two tests would be valid; the fixed-aspect test should still have predictable output values, so could use a similar approach to this test. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:154: if (task_runner_->BelongsToCurrentThread()) { Also, is there any way that MainFrameWasResized() could be invoked when task_runner_ is null? e.g. could it still arrive after Stop() has been called? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:154: if (task_runner_->BelongsToCurrentThread()) { On 2015/05/14 01:21:33, miu wrote: > On 2015/05/13 19:05:09, Wez wrote: > > So the task runner is sometimes UI thread and sometimes not...? > > tl;dr: This is the pattern decided upon, based on previous design/impl > decisions. > > WebContentsTracker is shared by both WebContentsVideoCaptureDevice and > WebContentsAudioInputStream. The former uses this class exclusively on the UI > thread, while the latter uses it from a separate media thread. The "tracking" > and "observing" logic is the same for both. > > While, granted, the WCAudioInputStream won't ever use this new "resize > observing" logic, I wanted to remain consistent with the threading model of the > rest of this class. Understood. The BelongsToCurrentThread() test and call is effectively a shortcut optimization - can we make it an early exit rather than if...else? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:159: base::Bind(&WebContentsTracker::MaybeDoResizeCallback, this, rwh)); What guarantees that the |rwh| will remain valid by the time this task is processed? We pass the RWH into the |resize_callback| - are we just using it as an identifier? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.h (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.h:45: typedef base::Callback<void(RenderWidgetHost* rwh)> ChangeCallback; As noted in the MaybeDoResizeCallback() impl, this doesn't look safe - in the case that Start() etc are called on a thread other than the UI thread, ChangeCallbacks are dispatched asynchronously, so there is no guarantee that the RWH still exists by the time that the callback task is run. The callback signature should therefore take a void* rather than a typed pointer, to protect against recipients accidentally trying to use the pointer to actually access RWH members. Looking at the WebContentsCaptureMachine, it looks like it's already (ab)using the RWH pointer in the callback to poke into RWH structures, so that will need to be fixed. :( https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.h:64: // method, which should be the same thread that calls Start()/Stop(). Calling nit: Suggest clarifying that this must be called from the same thread as Start/Stop, and _then_ that the |callback| will be called on that thread. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_video_capture_device.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device.cc:589: // updates, rather than just using the max frame size. This value here nit: "This value here"... ? :P https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device.cc:590: // determines the size of the fullscreen-within-tab widget. Bug # for that change? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:178: rwh_->WasResized(); nit: Reimpl this in terms of SetBounds() via gfx::Rect(Size)? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:229: // member. Bug # for that change? Looking at the code, doesn't seem there's a sound reason; the rwh parameter is cast to a RWHImpl and stored in |rwh_|, which seems perfectly safe to cast back to plain old RWH... https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:462: gfx::Size expected_frame_size_; This is a misleading name, since it's not the actual expected frame size. :P Generally speaking, it's preferable to have tests explicitly list out the values that they expect rather than replicating production size-calculation logic, which is basically what you're doing above in OnIncomingCapturedVideoFrame, IIUC.
Addressed round 2 comments. PTAL. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/content_video_capture_device_core.cc (left): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/content_video_capture_device_core.cc:248: // will convert to) YUV420. On 2015/05/14 01:44:54, Wez wrote: > Do we need to worry about the I420 even-size limitations here? No. As part of this change I realized we were writing a lot of redundant "make even" code all over the place. I've eliminated it in most places throughout these files. There are only two code points that still do it (the ones where an ARGB frame is converted to an I420 frame). I ran a ton of manual tests to double-check everything end-to-end is working correctly. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:357: // changed at least twice while capturing. On 2015/05/14 01:44:55, Wez wrote: > nit: Why is this not a loop? I originally wrote this test by forking the existing one (ScreenResolutionChangeVariableResolution). I've fixed all the tests to use a loop. > Why do we care that the source frame size has changed at least twice, versus > just once? Elaborated in code comments. > If the requirement is that it has changed at least twice, could we simply > compare received frame sizes between OnIncomingCapturedData() invocations? Yes. Thinking about this a bit, I've ripped out the existing "vector of sizes" approach, and replaced it with a simple FrameChecker helper class/method. I've done this for the three tests that correspond to each of the three possible resolution change policies. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:367: for (const media::VideoCaptureFormat& f : formats) { On 2015/05/14 01:44:55, Wez wrote: > nit: Could this be: > > for (auto format : formats) > > ? Reply 1: It could, but the style guide suggests only to use it where it would help readability (e.g., huge-string template class types, like iterators). IMO, it's more readable to provide the exact, known type here. Reply 2: No longer applicable since I've replaced this code. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:377: EXPECT_EQ(2u, distinct_sizes_seen.size()); On 2015/05/14 01:44:55, Wez wrote: > Is there something about the mock capturer that causes it to output two specific > sets of dimensions? Done. Elaborated on this in code comments. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:378: for (const gfx::Size& s : distinct_sizes_seen) On 2015/05/14 01:44:55, Wez wrote: > nit: auto here? No longer applicable since I've replaced this code. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:441: EXPECT_FALSE(have_seen_other_frame_size); On 2015/05/14 01:44:55, Wez wrote: > nit: The fixed aspect-ratio test collates a list of the sizes seen and checks > e.g. that there are exactly two of them - you could do the same here, plus test > that the two test frame sizes are contained in the list, and the resulting code > looks like it'd be more concise? > > Either way, it seems that using the same basic approach between the two tests > would be valid; the fixed-aspect test should still have predictable output > values, so could use a similar approach to this test. Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:154: if (task_runner_->BelongsToCurrentThread()) { On 2015/05/14 01:44:55, Wez wrote: > Also, is there any way that MainFrameWasResized() could be invoked when > task_runner_ is null? e.g. could it still arrive after Stop() has been called? No. It is set exactly once, when Start() is called, and before the WebContentsObserver starts observing the WebContents. It is never changed thereafter, even if Stop() is called. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:154: if (task_runner_->BelongsToCurrentThread()) { On 2015/05/14 01:44:55, Wez wrote: > can we make it an early exit rather than if...else? This seems like a matter of preference. I like the way the if...else reads since the code indentation helps to make it clear there are exactly two paths to take. Is there a particular reason you prefer the "early exit" style? https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:159: base::Bind(&WebContentsTracker::MaybeDoResizeCallback, this, rwh)); On 2015/05/14 01:44:55, Wez wrote: > What guarantees that the |rwh| will remain valid by the time this task is > processed? We pass the RWH into the |resize_callback| - are we just using it as > an identifier? Excellent point. I looked again at the other code in WebContentsTracker that does this. It turns out that the RWH pointer is only dereferenced by WCVideoCaptureDevice, which uses the pointer in the same task on the same UI thread, so it will always be valid. WCAudioInputStream never dereferences the pointer: It only checks whether the pointer is null to determine whether tracking has stopped (i.e., the tab was closed). So, as is, the code is correct everywhere. Though, I understand this usage pattern isn't obvious. I've done two things to aid future development: 1. Discussed this in the code comments in the header file where WebContentsTracker::ChangeCallback is declared. 2. Added a InvalidatePointerForDebugBuilds() function in the .cc file that, for debug builds, will map a non-null pointer to an invalid non-null pointer to ensure we see a crash when the pointer is misused. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.h (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.h:45: typedef base::Callback<void(RenderWidgetHost* rwh)> ChangeCallback; On 2015/05/14 01:44:55, Wez wrote: > As noted in the MaybeDoResizeCallback() impl, this doesn't look safe - in the > case that Start() etc are called on a thread other than the UI thread, > ChangeCallbacks are dispatched asynchronously, so there is no guarantee that the > RWH still exists by the time that the callback task is run. Agreed about the safety concern. See my response to your other comment. I think the remainder of your comment here was based on your assumption that dereferencing the RWH pointer is always dangerous. I've added code comments here to explain when it is safe and confirmed that it is really only being used when safe to do so. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.h:64: // method, which should be the same thread that calls Start()/Stop(). Calling On 2015/05/14 01:44:55, Wez wrote: > nit: Suggest clarifying that this must be called from the same thread as > Start/Stop, and _then_ that the |callback| will be called on that thread. Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_video_capture_device.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device.cc:589: // updates, rather than just using the max frame size. This value here On 2015/05/14 01:44:56, Wez wrote: > nit: "This value here"... ? :P Removed. I meant that the frame size we get here and manipulate below is what determines the size of the fullscreen-within-tab widget. However, the shorter TODO comment + bug reference should suffice. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device.cc:590: // determines the size of the fullscreen-within-tab widget. On 2015/05/14 01:44:56, Wez wrote: > Bug # for that change? Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:178: rwh_->WasResized(); On 2015/05/14 01:44:56, Wez wrote: > nit: Reimpl this in terms of SetBounds() via gfx::Rect(Size)? Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:183: rwh_->WasResized(); FYI--I found a bug, where WasResized() wasn't notifying the WebContents as expected because our test context doesn't spawn a render process. Therefore, we have to notify the WebContentsImpl ourselves. I added this to the SimulateSourceSizeChange() method below. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:229: // member. On 2015/05/14 01:44:56, Wez wrote: > Bug # for that change? Done. http://crbug.com/487939 FYI--I reworked a few things around the frame size checking and didn't need to access the RWH from this class anymore. > Looking at the code, doesn't seem there's a sound reason; the rwh parameter is > cast to a RWHImpl and stored in |rwh_|, which seems perfectly safe to cast back > to plain old RWH... I suspect it was just an oversight. However, I'd want to first make sure there isn't code anywhere that purposely interprets a nullptr return value as "Do some other behavior." https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:357: void SetExpectedFrameSizeProperties(const gfx::Size& max_size, FYI--This was moved into StubClientObserver, since there are threading issues and also delays in frame delivery after the source color or size are changed. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:462: gfx::Size expected_frame_size_; On 2015/05/14 01:44:56, Wez wrote: > This is a misleading name, since it's not the actual expected frame size. :P > > Generally speaking, it's preferable to have tests explicitly list out the values > that they expect rather than replicating production size-calculation logic, > which is basically what you're doing above in OnIncomingCapturedVideoFrame, > IIUC. Done.
https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:37: case media::RESOLUTION_POLICY_LAST: On 2015/05/14 01:21:32, miu wrote: > On 2015/05/13 19:05:08, Wez wrote: > > Won't this raise compilation errors since you're handling both LAST and > whatever > > the other name for the last value is? > > This is the "meaningless" enum value that only exists for the IPC layer to check > for a valid enum range. In other words, this switch statement has a case for > exactly one of each possible enum value. Dang, hoisted by my own petard! I thought the IPC "last" value was the same as the highest of the actual real values (i.e. actually the highest value, not one-more-than-highest), hence the comment. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:40: return gfx::Size(1, 1); On 2015/05/14 01:21:32, miu wrote: > On 2015/05/13 19:05:08, Wez wrote: > > nit: Can you get rid of this if you add a return to the LAST case? Then all > your > > cases have clear, explicit results. > > MSVC++ compiler complains, unfortunately, even though it's obvious the code > point couldn't be reached. Unless you're aware they've fixed this problem > recently? > > I'll give it a shot and see what happens on the trybots. I thought that VS2010 and above got this right, provided you take care to handle all the enum values, but I couldn't swear to that. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:113: void CaptureResolutionEvaluator::UpdateForNewCapabilityLimit(int num_pixels) { On 2015/05/14 01:21:32, miu wrote: > On 2015/05/13 19:05:08, Wez wrote: > > Is the rationale for including this in this CL rather than in the follow up CL > > just that the calling code wants to call it even though it's not yet > > implemented? > > No. I decided to add it in as a skeleton method so that it's clear in this > change why I'm going to all the trouble of adding a whole separate class to > encapsulate the resolution computation logic. OK; in that case I'd remove it and just add it in the CL where it gets implemented - your CL description can explain that breaking out this class is ground-work. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:12: On 2015/05/14 01:21:32, miu wrote: > On 2015/05/13 19:05:09, Wez wrote: > > nit: Don't think you need an empty namespace since const into will implicitly > > have static linkage, below? > > I don't want to export these symbols. I could remove the anonymous namespace > and add the static keyword, but that's just a matter of taste. ;-) My point is that you neither need the namespace nor the static keyword - const variables at global scope are implicitly static, AFAIK. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:154: if (task_runner_->BelongsToCurrentThread()) { On 2015/05/14 21:12:26, miu wrote: > On 2015/05/14 01:44:55, Wez wrote: > > can we make it an early exit rather than if...else? > > This seems like a matter of preference. I like the way the if...else reads > since the code indentation helps to make it clear there are exactly two paths to > take. Is there a particular reason you prefer the "early exit" style? I prefer early-exit because you could in principle have this code _always_ use PostTask() - calling MaybeDoResizeCallback directly is a short-cut optimization, basically. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:159: base::Bind(&WebContentsTracker::MaybeDoResizeCallback, this, rwh)); On 2015/05/14 21:12:26, miu wrote: > On 2015/05/14 01:44:55, Wez wrote: > > What guarantees that the |rwh| will remain valid by the time this task is > > processed? We pass the RWH into the |resize_callback| - are we just using it > as > > an identifier? > > Excellent point. I looked again at the other code in WebContentsTracker that > does this. It turns out that the RWH pointer is only dereferenced by > WCVideoCaptureDevice, which uses the pointer in the same task on the same UI > thread, so it will always be valid. WCAudioInputStream never dereferences the > pointer: It only checks whether the pointer is null to determine whether > tracking has stopped (i.e., the tab was closed). > > So, as is, the code is correct everywhere. Though, I understand this usage > pattern isn't obvious. I've done two things to aid future development: > > 1. Discussed this in the code comments in the header file where > WebContentsTracker::ChangeCallback is declared. > > 2. Added a InvalidatePointerForDebugBuilds() function in the .cc file that, for > debug builds, will map a non-null pointer to an invalid non-null pointer to > ensure we see a crash when the pointer is misused. Given the usage by the two consumers, could we instead switch to void* and update the WebContentsVideoCaptureDevice to hold its own copy of the RWH pointer, so that it's not vulnerable to crashing if someone decides to change the threading model in future? AFAICT it only uses the RWH to pull out the process & routing Id, to store in the ContentCaptureSubscription (see the ctor), so could we pass those in the callback instead of the RWH itself? https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:282: static void ExpectAcceptableSize(const media::VideoCaptureFormat& format) { Why is this in a struct rather than being a static global function(or some kind of gtest matcher/verifier..?) https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:398: class FormatChecker { Seems that this could be turned into a parameterized checker, with the expected two sizes supplied to the ctor? https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:94: RenderWidgetHost* InvalidatePointerInDebugBuilds(RenderWidgetHost* rwh) { See my other comments on how to avoid the need for this hack. :) https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.h (right): https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.h:48: // this pointer could be invalid at the time the callback is run. This warning makes ThreadingKitty sad. >.<
https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:40: return gfx::Size(1, 1); On 2015/05/15 00:55:42, Wez wrote: > On 2015/05/14 01:21:32, miu wrote: > > On 2015/05/13 19:05:08, Wez wrote: > > > nit: Can you get rid of this if you add a return to the LAST case? Then all > > your > > > cases have clear, explicit results. > > > > MSVC++ compiler complains, unfortunately, even though it's obvious the code > > point couldn't be reached. Unless you're aware they've fixed this problem > > recently? > > > > I'll give it a shot and see what happens on the trybots. > > I thought that VS2010 and above got this right, provided you take care to handle > all the enum values, but I couldn't swear to that. Nope. The compile of patch set 4 (where I tried it our way) failed to compile on the win8_chromium_rel trybot with: FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma/gomacc "E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\content\browser\media\capture\content_browser.capture_resolution_chooser.obj.rsp /c ..\..\content\browser\media\capture\capture_resolution_chooser.cc /Foobj\content\browser\media\capture\content_browser.capture_resolution_chooser.obj /Fdobj\content\content_browser.cc.pdb e:\b\build\slave\win\build\src\content\browser\media\capture\capture_resolution_chooser.cc(41) : error C2220: warning treated as error - no 'object' file generated e:\b\build\slave\win\build\src\content\browser\media\capture\capture_resolution_chooser.cc(41) : warning C4715: 'content::`anonymous namespace'::ComputeMinimumCaptureSize' : not all control paths return a value ninja: build stopped: subcommand failed. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator.cc:113: void CaptureResolutionEvaluator::UpdateForNewCapabilityLimit(int num_pixels) { On 2015/05/15 00:55:42, Wez wrote: > On 2015/05/14 01:21:32, miu wrote: > > On 2015/05/13 19:05:08, Wez wrote: > > > Is the rationale for including this in this CL rather than in the follow up > CL > > > just that the calling code wants to call it even though it's not yet > > > implemented? > > > > No. I decided to add it in as a skeleton method so that it's clear in this > > change why I'm going to all the trouble of adding a whole separate class to > > encapsulate the resolution computation logic. > > OK; in that case I'd remove it and just add it in the CL where it gets > implemented - your CL description can explain that breaking out this class is > ground-work. Done. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:12: On 2015/05/15 00:55:42, Wez wrote: > My point is that you neither need the namespace nor the static keyword - const > variables at global scope are implicitly static, AFAIK. They are static, but the symbol is public in the compilation unit. Meaning, there would be a content::kMaxFrameWidth, etc. that other compilation units may erroneously reference and this would unexpectedly resolve at link time without error. Instead, if other modules declare but do not define kMaxFrameWidth, it should result in a linker error. Said another way, my intention is that kMaxFrameWidth and friends should not be visible/accessible outside of this module. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:154: if (task_runner_->BelongsToCurrentThread()) { On 2015/05/15 00:55:42, Wez wrote: > On 2015/05/14 21:12:26, miu wrote: > > On 2015/05/14 01:44:55, Wez wrote: > > > can we make it an early exit rather than if...else? > > > > This seems like a matter of preference. I like the way the if...else reads > > since the code indentation helps to make it clear there are exactly two paths > to > > take. Is there a particular reason you prefer the "early exit" style? > > I prefer early-exit because you could in principle have this code _always_ use > PostTask() - calling MaybeDoResizeCallback directly is a short-cut optimization, > basically. Done. Feels like we're bike shedding here. ;-) https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:159: base::Bind(&WebContentsTracker::MaybeDoResizeCallback, this, rwh)); On 2015/05/15 00:55:42, Wez wrote: > On 2015/05/14 21:12:26, miu wrote: > > On 2015/05/14 01:44:55, Wez wrote: > > > What guarantees that the |rwh| will remain valid by the time this task is > > > processed? We pass the RWH into the |resize_callback| - are we just using it > > as > > > an identifier? > > > > Excellent point. I looked again at the other code in WebContentsTracker that > > does this. It turns out that the RWH pointer is only dereferenced by > > WCVideoCaptureDevice, which uses the pointer in the same task on the same UI > > thread, so it will always be valid. WCAudioInputStream never dereferences the > > pointer: It only checks whether the pointer is null to determine whether > > tracking has stopped (i.e., the tab was closed). > > > > So, as is, the code is correct everywhere. Though, I understand this usage > > pattern isn't obvious. I've done two things to aid future development: > > > > 1. Discussed this in the code comments in the header file where > > WebContentsTracker::ChangeCallback is declared. > > > > 2. Added a InvalidatePointerForDebugBuilds() function in the .cc file that, > for > > debug builds, will map a non-null pointer to an invalid non-null pointer to > > ensure we see a crash when the pointer is misused. > > Given the usage by the two consumers, could we instead switch to void* and > update the WebContentsVideoCaptureDevice to hold its own copy of the RWH > pointer, so that it's not vulnerable to crashing if someone decides to change > the threading model in future? > > AFAICT it only uses the RWH to pull out the process & routing Id, to store in > the ContentCaptureSubscription (see the ctor), so could we pass those in the > callback instead of the RWH itself? Done, but I got rid of the pointer altogether. I realized the client could always call WebContentsTracker::GetTargetRenderWidgetHost() on the UI thread if it needed access to the object. https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:282: static void ExpectAcceptableSize(const media::VideoCaptureFormat& format) { On 2015/05/15 00:55:42, Wez wrote: > Why is this in a struct rather than being a static global function(or some kind > of gtest matcher/verifier..?) No particular reason, other than it's only used within, and highly specialized in purpose to this function. No longer applicable, since I now use one FormatChecker for all three tests. https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... content/browser/media/capture/desktop_capture_device_unittest.cc:398: class FormatChecker { On 2015/05/15 00:55:42, Wez wrote: > Seems that this could be turned into a parameterized checker, with the expected > two sizes supplied to the ctor? Done. Good point. https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:94: RenderWidgetHost* InvalidatePointerInDebugBuilds(RenderWidgetHost* rwh) { On 2015/05/15 00:55:42, Wez wrote: > See my other comments on how to avoid the need for this hack. :) Acknowledged. https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.h (right): https://codereview.chromium.org/1135823004/diff/80001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.h:48: // this pointer could be invalid at the time the callback is run. On 2015/05/15 00:55:42, Wez wrote: > This warning makes ThreadingKitty sad. >.< Acknowledged.
lgtm https://codereview.chromium.org/1135823004/diff/100001/content/browser/media/... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/100001/content/browser/media/... content/browser/media/capture/web_contents_tracker.cc:99: MaybeDoCallback(!!rwh); rwh != nullptr here and below; !!rwh looks weird!
The CQ bit was checked by miu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1135823004/#ps140001 (title: "REBASE")
https://codereview.chromium.org/1135823004/diff/100001/content/browser/media/... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/100001/content/browser/media/... content/browser/media/capture/web_contents_tracker.cc:99: MaybeDoCallback(!!rwh); On 2015/05/15 21:36:06, Wez wrote: > rwh != nullptr here and below; !!rwh looks weird! Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135823004/140001
https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/capture_resolution_evaluator_unittest.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/capture_resolution_evaluator_unittest.cc:12: On 2015/05/15 21:14:00, miu wrote: > On 2015/05/15 00:55:42, Wez wrote: > > My point is that you neither need the namespace nor the static keyword - const > > variables at global scope are implicitly static, AFAIK. > > They are static, but the symbol is public in the compilation unit. Meaning, > there would be a content::kMaxFrameWidth, etc. that other compilation units may > erroneously reference and this would unexpectedly resolve at link time without > error. Instead, if other modules declare but do not define kMaxFrameWidth, it > should result in a linker error. > > Said another way, my intention is that kMaxFrameWidth and friends should not be > visible/accessible outside of this module. Yes, I understand that - if they were _global_ consts then AFAIK they should be implicitly static, i.e. not visible outside this module - they're only visible w/out the anon namespace because they're not global, but appear inside the content namespace. https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... File content/browser/media/capture/web_contents_tracker.cc (right): https://codereview.chromium.org/1135823004/diff/20001/content/browser/media/c... content/browser/media/capture/web_contents_tracker.cc:159: base::Bind(&WebContentsTracker::MaybeDoResizeCallback, this, rwh)); On 2015/05/15 21:14:00, miu wrote: > On 2015/05/15 00:55:42, Wez wrote: > > On 2015/05/14 21:12:26, miu wrote: > > > On 2015/05/14 01:44:55, Wez wrote: > > > > What guarantees that the |rwh| will remain valid by the time this task is > > > > processed? We pass the RWH into the |resize_callback| - are we just using > it > > > as > > > > an identifier? > > > > > > Excellent point. I looked again at the other code in WebContentsTracker > that > > > does this. It turns out that the RWH pointer is only dereferenced by > > > WCVideoCaptureDevice, which uses the pointer in the same task on the same UI > > > thread, so it will always be valid. WCAudioInputStream never dereferences > the > > > pointer: It only checks whether the pointer is null to determine whether > > > tracking has stopped (i.e., the tab was closed). > > > > > > So, as is, the code is correct everywhere. Though, I understand this usage > > > pattern isn't obvious. I've done two things to aid future development: > > > > > > 1. Discussed this in the code comments in the header file where > > > WebContentsTracker::ChangeCallback is declared. > > > > > > 2. Added a InvalidatePointerForDebugBuilds() function in the .cc file that, > > for > > > debug builds, will map a non-null pointer to an invalid non-null pointer to > > > ensure we see a crash when the pointer is misused. > > > > Given the usage by the two consumers, could we instead switch to void* and > > update the WebContentsVideoCaptureDevice to hold its own copy of the RWH > > pointer, so that it's not vulnerable to crashing if someone decides to change > > the threading model in future? > > > > AFAICT it only uses the RWH to pull out the process & routing Id, to store in > > the ContentCaptureSubscription (see the ctor), so could we pass those in the > > callback instead of the RWH itself? > > Done, but I got rid of the pointer altogether. I realized the client could > always call WebContentsTracker::GetTargetRenderWidgetHost() on the UI thread if > it needed access to the object. Nice!
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/39254e67228717bbddf222614abd6cb9ba38608c Cr-Commit-Position: refs/heads/master@{#330232} |
