Refactor WebRtcVideoCapturerAdapter |
||||
Issue descriptionWebRtcVideoCapturerAdapter::IsScreencast will be ignored by WebRTC and CPU adaptation will always be enabled. This is because CPU adaptation is required for simulcast screenshare. This is fine because resolution will maintained for screenshare and for relevant content hints because they are being signaled to webrtc via VideoTrack here: https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc?sq=package:chromium&g=0&l=366 This sets sets degradation preference to maintain resolution and then sink wants will never be called with maximum pixels count. That change, however, will break some WebRtcVideoCapturerAdapter tests because they assume content hints are respected by VideoAdapter.
,
Jul 17
+hta@ as this sounds like it might break https://www.w3.org/TR/mst-content-hint/. This code path being removed uses WebRtcVideoCapturerAdapter::ShouldAdaptResolution() which is how content hints are implemented. You should probably not remove those tests either unless content hint code paths are otherwise covered. Please do not submit this removal without hta@'s approval.
,
Jul 17
xref, content hints are covered by issue 653531 .
,
Jul 17
Can you run the demo and check that it works after making your change? The demo is here: https://webrtc.github.io/samples/src/content/capture/video-contenthint/ You have to enable experimental flags to see any effect.
,
Jul 18
I can confirm that the demo works with the changes applied exactly as without them (detail video looks better, but more jumpy, motion video looks less detailed but more smooth). This is because the change only enables adaptation on all the content types. However, content hints are ultimately converted to degradation preference and webrtc doesn't scale down detail-video and doesn't reduce fps for motion-video.
,
Jul 18
Also, before the change webrtc didn't ever drop frames or scaled down on detail-video and screenshare. It was fine if we had only 5fps, but if CPU adaptation was ever needed, it didn't happen at all. Now degradation preference "maintain resolution" would be set and video adapter actually would kick in and drop some frames.
,
Jul 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/90665ce020b9c4959dd9ba4a838e10cd04ea2627 commit 90665ce020b9c4959dd9ba4a838e10cd04ea2627 Author: Ilya Nikolaevskiy <ilnik@chromium.org> Date: Wed Jul 18 07:55:28 2018 Disable failing WebRtcVideoCapturerAdapter tests Bug: 864529 Change-Id: I2393217288491a145139c56c7ec1512d2516da18 Reviewed-on: https://chromium-review.googlesource.com/1140315 Commit-Queue: Ilya Nikolaevskiy <ilnik@chromium.org> Reviewed-by: Sergey Ulanov <sergeyu@chromium.org> Cr-Commit-Position: refs/heads/master@{#575972} [modify] https://crrev.com/90665ce020b9c4959dd9ba4a838e10cd04ea2627/content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc
,
Jul 18
Thanks, that addresses it perfectly for me. I was just concerned about this case being missed.
,
Jul 23
For context, what i am trying to do is to enable cpu adaptation for screenshare (reduced framerate). My changes shouldn't break content hints. Before we just disabled all adaptation for screenshare and content hints piggybacked on that (signaling that detail realtime video is screenshare). However, simply removing that check doesn't break hints, but rather enables cpu adaptation to drop frames for detail video (which was impossible before). Content hints are propagated to both adapter and video track here: https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc?sq=package:chromium&g=0&l=364 From video track it ultimately ends up here: https://cs.chromium.org/chromium/src/third_party/webrtc/pc/rtpsender.cc?gsn=set_content_hint&g=0&l=379 In turn, this marks detail video as screenshare here: https://cs.chromium.org/chromium/src/third_party/webrtc/pc/rtpsender.cc?gsn=SetVideoSend&l=516 Which will force maintain_resolution degradation preference here: https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvideoengine.cc?gsn=SetVideoSend&g=0&l=1703 Because chrome always asks for balanced degradation preference here: https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/peerconnection/rtc_rtp_sender.cc?gsn=rtp_parameters_&g=0&l=384 https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/peerconnection/rtc_rtp_sender.cc?gsn=ToRtpParameters&l=230 So, the only situation which breaks content hints is when we receive both detail_video and maintain_frame-rate degradation preference (which is contradictory, right?) Maintain_resolution degradation preference will never pass max_pixel constraints to video adapter, and therefore, video will never be down scaled even if adapt() call is made in video capturer.
,
Jul 23
I think maintain framerate as degradationPreference should take precedence over the content hint, so that sounds correct to me. We are allowed to do other things with the content hint (change QP values springs to mind for instance) but not when they are in direct conflict with other settings.
,
Jul 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6c688a55956c3f6d5ffeeaef0a0098d7af868761 commit 6c688a55956c3f6d5ffeeaef0a0098d7af868761 Author: Ilya Nikolaevskiy <ilnik@chromium.org> Date: Thu Jul 26 06:27:13 2018 WebRtc Capturer Adapter: don't confuse screenshare and content hints TESTING=manual using https://webrtc.github.io/samples/src/content/capture/video-contenthint/ Bug: 864529 Change-Id: I7ca26888773922117848f2462cf03b7d9e28295a Reviewed-on: https://chromium-review.googlesource.com/1146564 Reviewed-by: Harald Alvestrand <hta@chromium.org> Reviewed-by: Henrik Boström <hbos@chromium.org> Commit-Queue: Ilya Nikolaevskiy <ilnik@chromium.org> Cr-Commit-Position: refs/heads/master@{#578218} [modify] https://crrev.com/6c688a55956c3f6d5ffeeaef0a0098d7af868761/content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc [modify] https://crrev.com/6c688a55956c3f6d5ffeeaef0a0098d7af868761/content/renderer/media/webrtc/webrtc_video_capturer_adapter.h
,
Jul 30
Now there's no confusion between screenshare and content hints in webrtc video capturer.
,
Aug 27
,
Dec 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f32bbd8e61c3256c23f5f16826b8c7ae8d1dc233 commit f32bbd8e61c3256c23f5f16826b8c7ae8d1dc233 Author: Niels Möller <nisse@chromium.org> Date: Fri Dec 14 09:26:55 2018 Delete remnants of ContentHint handling in WebRtcVideoCapturerAdapter The |content_hint_| member is unused since https://chromium-review.googlesource.com/c/chromium/src/+/1146564/. Deletes wiring in MediaStreamVideoWebRtcSink, as a preparation for replacing WebRtcVideoCapturerAdapter (a subclass of cricket::VideoCapturer) with an implementation of webrtc::VideoTrackSourceInterface. Bug: 864529 , webrtc:6353 Change-Id: I48972c63a6c69ceb859fb11fcad98d6faefac524 Reviewed-on: https://chromium-review.googlesource.com/c/1375720 Reviewed-by: Sergey Ulanov <sergeyu@chromium.org> Commit-Queue: Niels Möller <nisse@chromium.org> Cr-Commit-Position: refs/heads/master@{#616620} [modify] https://crrev.com/f32bbd8e61c3256c23f5f16826b8c7ae8d1dc233/content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc [modify] https://crrev.com/f32bbd8e61c3256c23f5f16826b8c7ae8d1dc233/content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc [modify] https://crrev.com/f32bbd8e61c3256c23f5f16826b8c7ae8d1dc233/content/renderer/media/webrtc/webrtc_video_capturer_adapter.h [modify] https://crrev.com/f32bbd8e61c3256c23f5f16826b8c7ae8d1dc233/content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc |
||||
►
Sign in to add a comment |
||||
Comment 1 by ilnik@chromium.org
, Jul 17