Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2585313002:
Propagate MediaStreamTrack video content hints. (Closed)
|
Created:
4 years ago by pbos Modified:
4 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPropagate MediaStreamTrack video content hints.
Fluid/detailed overrides the screencast settings inside webrtc which
disables downscaling and configures libvpx differently.
Also disables resolution adaptation on the Chromium side corresponding
to the IsScreencast setting.
BUG=chromium:653531
R=emircan@chromium.org, mcasas@chromium.org
Committed: https://crrev.com/49b5535454c4a02113b41d7f7dc5a597461270be
Cr-Commit-Position: refs/heads/master@{#439643}
Patch Set 1 #Patch Set 2 : add tests + fix compile #
Total comments: 8
Patch Set 3 : remove else from else-if #
Total comments: 11
Patch Set 4 : review feedback + more test cases #Patch Set 5 : fix test #
Messages
Total messages: 33 (15 generated)
PTAL
The CQ bit was checked by pbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
add tests + fix compile
Description was changed from ========== Propagate MediaStreamTrack video content hints. Fluid/detailed overrides the screencast settings inside webrtc which disables downscaling and configures libvpx differently. Also disables resolution adaptation on the Chromium side corresponding to the IsScreencast setting. BUG=chromium:653531 R=mcasas@chromium.org ========== to ========== Propagate MediaStreamTrack video content hints. Fluid/detailed overrides the screencast settings inside webrtc which disables downscaling and configures libvpx differently. Also disables resolution adaptation on the Chromium side corresponding to the IsScreencast setting. BUG=chromium:653531 R=emircan@chromium.org, mcasas@chromium.org ==========
pbos@chromium.org changed reviewers: + emircan@chromium.org
PTAL, emircan@ can you do the review and mcasas@ can stamp later?
The CQ bit was checked by pbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right): https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:69: NOTREACHED(); Why are these hints causing a NOTREACHED() ? IOW why are they a bad thing? https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:325: video_track_->set_content_hint(TranslateContentHint(track.contentHint())); Isn't this repeating what's done in l.313? https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:358: source_adapter_->SetContentHint(content_hint); IIUC,WebRtcVideoCapturerAdapter::SetContentHint() checks that the update happens in jingle's worker thread, correct? Where are we here? (if this is the worker thread, can we s/thread_checker_/worker_thread_checker_/ ? https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:321: } else if (content_hint_ == nit: no need for else{} after if(){ return }
remove else from else-if
https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right): https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:69: NOTREACHED(); On 2016/12/19 19:32:05, mcasas wrote: > Why are these hints causing a NOTREACHED() ? > IOW why are they a bad thing? They should never be settable on a video track. This is filtered out in blink's javascript layer, so audio hints should never be set on a video track. https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:325: video_track_->set_content_hint(TranslateContentHint(track.contentHint())); On 2016/12/19 19:32:05, mcasas wrote: > Isn't this repeating what's done in l.313? Nope, l.313 is for the adapter. https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:358: source_adapter_->SetContentHint(content_hint); On 2016/12/19 19:32:05, mcasas wrote: > IIUC,WebRtcVideoCapturerAdapter::SetContentHint() checks > that the update happens in jingle's worker thread, correct? > Where are we here? (if this is the worker thread, can we > s/thread_checker_/worker_thread_checker_/ ? We are on the renderer (signalling) thread here. SetContentHint does a thread hop to SetContentHintOnWorker. https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2585313002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:321: } else if (content_hint_ == On 2016/12/19 19:32:05, mcasas wrote: > nit: no need for else{} after if(){ return } Done.
https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right): https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:62: webrtc::VideoTrackInterface::ContentHint TranslateContentHint( ContentHintTypeToWebrtcContentHint()? I have seen XToY() naming more commonly used in chrome. https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:312: bool WebRtcVideoCapturerAdapter::IsScreencast() const { Can you merge these functions then? It would be easier to read without double negating. https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:103: gfx::Size size(kInputWidth, kInputHeight); const https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:160: // Non-screenshare adapter should not adapt frames when detailed is set. Can you parameterize these tests using GetParam() so you have all the combinations covered? There are more cases AFAIU, i.e. non-screenshare with fluid.
review feedback + more test cases
PTAL https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right): https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:62: webrtc::VideoTrackInterface::ContentHint TranslateContentHint( On 2016/12/19 21:14:27, emircan wrote: > ContentHintTypeToWebrtcContentHint()? I have seen XToY() naming more commonly > used in chrome. Done. https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:312: bool WebRtcVideoCapturerAdapter::IsScreencast() const { On 2016/12/19 21:14:27, emircan wrote: > Can you merge these functions then? It would be easier to read without double > negating. ShouldAdaptResolution makes more sense because the decision isn't really just whether it's screencast or not. This overrides VideoCapturer::IsScreencast() which is really just this decision. I've put a comment + TODO in there instead. WDYT? https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:103: gfx::Size size(kInputWidth, kInputHeight); On 2016/12/19 21:14:27, emircan wrote: > const Done. https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:160: // Non-screenshare adapter should not adapt frames when detailed is set. On 2016/12/19 21:14:27, emircan wrote: > Can you parameterize these tests using GetParam() so you have all the > combinations covered? There are more cases AFAIU, i.e. non-screenshare with > fluid. I added the corresponding transitions instead, hope that's alright. I prefer them named because they're more descriptive than GetParam.
lgtm https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:312: bool WebRtcVideoCapturerAdapter::IsScreencast() const { On 2016/12/19 21:42:26, pbos wrote: > On 2016/12/19 21:14:27, emircan wrote: > > Can you merge these functions then? It would be easier to read without double > > negating. > > ShouldAdaptResolution makes more sense because the decision isn't really just > whether it's screencast or not. This overrides VideoCapturer::IsScreencast() > which is really just this decision. I've put a comment + TODO in there instead. > WDYT? That's fine. Renaming that interface definitely belongs to a separate CL. https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:160: // Non-screenshare adapter should not adapt frames when detailed is set. On 2016/12/19 21:42:26, pbos wrote: > On 2016/12/19 21:14:27, emircan wrote: > > Can you parameterize these tests using GetParam() so you have all the > > combinations covered? There are more cases AFAIU, i.e. non-screenshare with > > fluid. > > I added the corresponding transitions instead, hope that's alright. I prefer > them named because they're more descriptive than GetParam. I would highly recommend it because you can easily make a combination of all the used cases, see https://cs.chromium.org/chromium/src/content/renderer/media/canvas_capture_ha.... It would make it much easier to maintain and add more since testing::combine is the perfect use case for you here. I will leave the decision to you.
RS lgtm
The CQ bit was checked by pbos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/2585313002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:160: // Non-screenshare adapter should not adapt frames when detailed is set. On 2016/12/19 22:37:53, emircan wrote: > On 2016/12/19 21:42:26, pbos wrote: > > On 2016/12/19 21:14:27, emircan wrote: > > > Can you parameterize these tests using GetParam() so you have all the > > > combinations covered? There are more cases AFAIU, i.e. non-screenshare with > > > fluid. > > > > I added the corresponding transitions instead, hope that's alright. I prefer > > them named because they're more descriptive than GetParam. > > I would highly recommend it because you can easily make a combination of all the > used cases, see > https://cs.chromium.org/chromium/src/content/renderer/media/canvas_capture_ha.... > It would make it much easier to maintain and add more since testing::combine is > the perfect use case for you here. I will leave the decision to you. Ack, will go with this now be because 2/5 parameters here are expectations, so I can't use test::combine straight out of the box.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
fix test
The CQ bit was checked by pbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2585313002/#ps80001 (title: "fix test")
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": 80001, "attempt_start_ts": 1482191211971180,
"parent_rev": "9270bfc68fffdb529d69bd8080fefc3c371da8bf", "commit_rev":
"7722c7a6890681d8b154a86d4aea44b4e698e762"}
Message was sent while issue was closed.
Description was changed from ========== Propagate MediaStreamTrack video content hints. Fluid/detailed overrides the screencast settings inside webrtc which disables downscaling and configures libvpx differently. Also disables resolution adaptation on the Chromium side corresponding to the IsScreencast setting. BUG=chromium:653531 R=emircan@chromium.org, mcasas@chromium.org ========== to ========== Propagate MediaStreamTrack video content hints. Fluid/detailed overrides the screencast settings inside webrtc which disables downscaling and configures libvpx differently. Also disables resolution adaptation on the Chromium side corresponding to the IsScreencast setting. BUG=chromium:653531 R=emircan@chromium.org, mcasas@chromium.org Review-Url: https://codereview.chromium.org/2585313002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Propagate MediaStreamTrack video content hints. Fluid/detailed overrides the screencast settings inside webrtc which disables downscaling and configures libvpx differently. Also disables resolution adaptation on the Chromium side corresponding to the IsScreencast setting. BUG=chromium:653531 R=emircan@chromium.org, mcasas@chromium.org Review-Url: https://codereview.chromium.org/2585313002 ========== to ========== Propagate MediaStreamTrack video content hints. Fluid/detailed overrides the screencast settings inside webrtc which disables downscaling and configures libvpx differently. Also disables resolution adaptation on the Chromium side corresponding to the IsScreencast setting. BUG=chromium:653531 R=emircan@chromium.org, mcasas@chromium.org Committed: https://crrev.com/49b5535454c4a02113b41d7f7dc5a597461270be Cr-Commit-Position: refs/heads/master@{#439643} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/49b5535454c4a02113b41d7f7dc5a597461270be Cr-Commit-Position: refs/heads/master@{#439643} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
