New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment
link

Issue 864529: Refactor WebRtcVideoCapturerAdapter

Reported by ilnik@chromium.org, Jul 17 2018 Project Member

Issue description

WebRtcVideoCapturerAdapter::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.
 

Comment 1 by ilnik@chromium.org, Jul 17 2018

I am removing the following tests:
WebRtcVideoCapturerAdapterTest.NonScreencastAdapterDoesNotAdaptContentHintDetail
WebRtcVideoCapturerAdapterTest.NonScreencastAdapterDoesNotAdaptContentHintText
WebRtcVideoCapturerAdapterTest.NonScreencastAdapterAdaptsContentHintFluid
WebRtcVideoCapturerAdapterTest.RespectsConstructionTimeContentHint 
WebRtcVideoCapturerAdapterTest.ScreencastAdapterAdaptsContentHintFluid WebRtcVideoCapturerAdapterTest.ScreencastAdapterDoesNotAdaptContentHintDetailed

Comment 2 by pbos@chromium.org, Jul 17 2018

Cc: hta@chromium.org
+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.

Comment 3 by pbos@chromium.org, Jul 17 2018

xref, content hints are covered by  issue 653531 .

Comment 4 by hta@webrtc.org, Jul 17 2018

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.

Comment 5 by ilnik@chromium.org, Jul 18 2018

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.

Comment 6 by ilnik@chromium.org, Jul 18 2018

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.

Comment 7 by bugdroid1@chromium.org, Jul 18 2018

Project Member
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

Comment 8 by pbos@chromium.org, Jul 18 2018

Thanks, that addresses it perfectly for me. I was just concerned about this case being missed.

Comment 9 by ilnik@chromium.org, Jul 23 2018

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.

Comment 10 by pbos@chromium.org, Jul 23 2018

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.

Comment 11 by bugdroid1@chromium.org, Jul 26 2018

Project Member
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

Comment 12 by ilnik@chromium.org, Jul 30 2018

Status: Fixed (was: Assigned)
Now there's no confusion between screenshare and content hints in webrtc video capturer.

Comment 13 by anatolid@chromium.org, Aug 27

Labels: M-70

Comment 14 by bugdroid1@chromium.org, Dec 14

Project Member
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