New issue
Advanced search Search tips

Issue 645907 link

Starred by 2 users
Status: Fixed
Owner:
pbos@chromium.org
Closed: Oct 2016
Cc:
tommi@chromium.org
mflodman@chromium.org
pbos@chromium.org
nisse@chromium.org
hta@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

googNoiseReduction constraint broken

Project Member Reported by nisse@chromium.org, Sep 12 2016 
Version: M54
OS: all

What steps will reproduce the problem?
(1) pass the googNoiseReduction constraint to getUserMedia

What is the expected output?

The constraint is applied, enabling or disabling denoising at the encoder.

What do you see instead?

Constraint is ignored, codec-dependent default is always used instead.

This was likely broken in cl https://codereview.chromium.org/1729683002, landed between M50 and M51 (thanks to pbos for digging).

Comment 1 by mflodman@chromium.org, Sep 12 2016

Cc: tommi@chromium.org mflodman@chromium.org

Comment 3 by pbos@chromium.org, Sep 15 2016 

nisse@ is this ready to be taken over from the Blink side?

Comment 4 by nisse@chromium.org, Sep 15 2016 

Yes, I hope that's all you need to wire it up again. Let me know if there are any problems.

Comment 5 by pbos@chromium.org, Sep 15 2016

Cc: nisse@chromium.org
Owner: pbos@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/36f03f6131cb668e175cadd2d0407d09b33c199c

commit 36f03f6131cb668e175cadd2d0407d09b33c199c
Author: pbos <pbos@chromium.org>
Date: Mon Sep 26 18:45:13 2016

Implement and use VideoTrackSource directly.

Removes dependency on PeerConnectionDependencyFactory for creating a
VideoTrackSource. Also implements VideoTrackSource::needs_denoising()
and wires it up to googNoiseReduction from getUserMedia().

This has less of an explicit dependency on cricket::VideoCapturer, and
the intent is to remove the dependency on cricket::VideoCapturer
completely as soon as the logic for AdaptFrame has been extracted.

BUG= chromium:645907 
R=nisse@chromium.org, hta@chromium.org, perkj@chromium.org

Review-Url: https://codereview.chromium.org/2356663002
Cr-Commit-Position: refs/heads/master@{#420947}

[modify] https://crrev.com/36f03f6131cb668e175cadd2d0407d09b33c199c/content/renderer/media/webrtc/media_stream_remote_video_source_unittest.cc
[modify] https://crrev.com/36f03f6131cb668e175cadd2d0407d09b33c199c/content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc
[modify] https://crrev.com/36f03f6131cb668e175cadd2d0407d09b33c199c/content/renderer/media/webrtc/media_stream_video_webrtc_sink.h
[modify] https://crrev.com/36f03f6131cb668e175cadd2d0407d09b33c199c/content/renderer/media/webrtc/mock_peer_connection_dependency_factory.cc
[modify] https://crrev.com/36f03f6131cb668e175cadd2d0407d09b33c199c/content/renderer/media/webrtc/mock_peer_connection_dependency_factory.h
[modify] https://crrev.com/36f03f6131cb668e175cadd2d0407d09b33c199c/content/renderer/media/webrtc/peer_connection_dependency_factory.cc
[modify] https://crrev.com/36f03f6131cb668e175cadd2d0407d09b33c199c/content/renderer/media/webrtc/peer_connection_dependency_factory.h

Comment 7 by pbos@chromium.org, Sep 27 2016 

Note: This is fixed in HEAD but still open until the code gets unittests.
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 10 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38db686694408c72809cf99a0d664a6d2b6c7495

commit 38db686694408c72809cf99a0d664a6d2b6c7495
Author: hta <hta@chromium.org>
Date: Mon Oct 10 21:38:25 2016

Add unit tests for webrtc_media_stream_adapter.cc

Also adds "googNoiseReduction" to the set of permitted
video constraints (needed for mandatory to be allowed).

BUG= 645907 

Review-Url: https://codereview.chromium.org/2393923002
Cr-Commit-Position: refs/heads/master@{#424252}

[modify] https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495/content/renderer/media/media_stream_constraints_util.cc
[modify] https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495/content/renderer/media/media_stream_video_source.cc
[modify] https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495/content/renderer/media/mock_media_stream_registry.cc
[modify] https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495/content/renderer/media/mock_media_stream_registry.h
[modify] https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495/content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc
[modify] https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495/content/renderer/media/webrtc/media_stream_video_webrtc_sink.h
[add] https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495/content/renderer/media/webrtc/media_stream_video_webrtc_sink_unittest.cc
[modify] https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495/content/test/BUILD.gn
[modify] https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp

Comment 9 by pbos@chromium.org, Oct 10 2016

Status: Fixed (was: Started)
hta@ I assume #8 closes it with enough tests, reopen otherwise.
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 24 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/66712b024f1e3490c1aecee431e0897c961c54fe

commit 66712b024f1e3490c1aecee431e0897c961c54fe
Author: nisse <nisse@webrtc.org>
Date: Mon Oct 24 07:22:31 2016

Revert of Add method cricket::VideoCapturer::NeedsDenoising, use in VideoCapturerTrackSource. (patchset #5 id:80001 of https://codereview.webrtc.org/2334683002/ )

Reason for revert:
This was a workaround to help Chrome wire up the googNoiseReduction constraint. However, it was fixed in a different way in Chrome, and this hack is not actually needed.

Original issue's description:
> Add method cricket::VideoCapturer::NeedsDenoising, use in VideoCapturerTrackSource.
>
> BUG= chromium:645907 
>
> Committed: https://crrev.com/0d14c6abba19295725519ce38105688ae0a62513
> Cr-Commit-Position: refs/heads/master@{#14219}

TBR=pbos@webrtc.org,hta@webrtc.org,perkj@webrtc.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= chromium:645907 

Review-Url: https://codereview.webrtc.org/2433293003
Cr-Commit-Position: refs/heads/master@{#14729}

[modify] https://crrev.com/66712b024f1e3490c1aecee431e0897c961c54fe/webrtc/api/videocapturertracksource.cc
[modify] https://crrev.com/66712b024f1e3490c1aecee431e0897c961c54fe/webrtc/api/videocapturertracksource_unittest.cc
[modify] https://crrev.com/66712b024f1e3490c1aecee431e0897c961c54fe/webrtc/media/base/fakevideocapturer.h
[modify] https://crrev.com/66712b024f1e3490c1aecee431e0897c961c54fe/webrtc/media/base/videocapturer.h

Comment 11 by anatolid@chromium.org, Nov 29 2016

Labels: M-56
►

Sign in to add a comment