Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2334683002:
Add method cricket::VideoCapturer::NeedsDenoising, use in VideoCapturerTrackSource. (Closed)
|
Created:
4 years, 3 months ago by nisse-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, perkj_webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd method cricket::VideoCapturer::NeedsDenoising, use in VideoCapturerTrackSource.
BUG=chromium:645907
Committed: https://crrev.com/0d14c6abba19295725519ce38105688ae0a62513
Cr-Commit-Position: refs/heads/master@{#14219}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add tests for needs_denoising. #
Total comments: 6
Patch Set 3 : Address pbos' comments. #
Total comments: 8
Patch Set 4 : Delete comment about Chrome. #Patch Set 5 : TODO comment explaining the NeedsDenoising hack. #
Messages
Total messages: 30 (10 generated)
nisse@webrtc.org changed reviewers: + hta@webrtc.org, pbos@webrtc.org
PTAL. At least, it's pretty close to a minimal change.
https://codereview.webrtc.org/2334683002/diff/1/webrtc/api/videocapturertrack... File webrtc/api/videocapturertracksource.cc (right): https://codereview.webrtc.org/2334683002/diff/1/webrtc/api/videocapturertrack... webrtc/api/videocapturertracksource.cc:323: // Get default from the capturer, overridden by constraints, if any. Can you add test coverage for this? That would also have caught that there's no way to override ::NeedsDenoising(). https://codereview.webrtc.org/2334683002/diff/1/webrtc/media/base/videocaptur... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2334683002/diff/1/webrtc/media/base/videocaptur... webrtc/media/base/videocapturer.h:179: rtc::Optional<bool> NeedsDenoising() const { virtual, since you want this to be overridden.
https://codereview.webrtc.org/2334683002/diff/1/webrtc/api/videocapturertrack... File webrtc/api/videocapturertracksource.cc (right): https://codereview.webrtc.org/2334683002/diff/1/webrtc/api/videocapturertrack... webrtc/api/videocapturertracksource.cc:323: // Get default from the capturer, overridden by constraints, if any. On 2016/09/12 14:50:45, pbos-webrtc wrote: > Can you add test coverage for this? That would also have caught that there's no > way to override ::NeedsDenoising(). I can override it in FakeVideoCapturer. But not sure what to test. Maybe it makes sense to add a unit test in videocapturertracksource that checks that it is propagated there, to VideoTrackSource::needs_denoising? https://codereview.webrtc.org/2334683002/diff/1/webrtc/media/base/videocaptur... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2334683002/diff/1/webrtc/media/base/videocaptur... webrtc/media/base/videocapturer.h:179: rtc::Optional<bool> NeedsDenoising() const { On 2016/09/12 14:50:45, pbos-webrtc wrote: > virtual, since you want this to be overridden. Fixed in next upload. Good catch.
Commenting... https://codereview.webrtc.org/2334683002/diff/1/webrtc/api/videocapturertrack... File webrtc/api/videocapturertracksource.cc (right): https://codereview.webrtc.org/2334683002/diff/1/webrtc/api/videocapturertrack... webrtc/api/videocapturertracksource.cc:323: // Get default from the capturer, overridden by constraints, if any. On 2016/09/13 09:39:10, nisse-webrtc wrote: > On 2016/09/12 14:50:45, pbos-webrtc wrote: > > Can you add test coverage for this? That would also have caught that there's > no > > way to override ::NeedsDenoising(). > > I can override it in FakeVideoCapturer. But not sure what to test. Maybe it > makes sense to add a unit test in videocapturertracksource that checks that it > is propagated there, to VideoTrackSource::needs_denoising? Yes, that's a good test - that the value propagates and is consistent with the request. A lower priority item-that-needs-doing is to be able to read back the value of "denoising" - whether it's off or on. This is needed for getSettings().
On 2016/09/13 09:46:27, hta-webrtc wrote: > Commenting... > > https://codereview.webrtc.org/2334683002/diff/1/webrtc/api/videocapturertrack... > File webrtc/api/videocapturertracksource.cc (right): > > https://codereview.webrtc.org/2334683002/diff/1/webrtc/api/videocapturertrack... > webrtc/api/videocapturertracksource.cc:323: // Get default from the capturer, > overridden by constraints, if any. > On 2016/09/13 09:39:10, nisse-webrtc wrote: > > On 2016/09/12 14:50:45, pbos-webrtc wrote: > > > Can you add test coverage for this? That would also have caught that there's > > no > > > way to override ::NeedsDenoising(). > > > > I can override it in FakeVideoCapturer. But not sure what to test. Maybe it > > makes sense to add a unit test in videocapturertracksource that checks that it > > is propagated there, to VideoTrackSource::needs_denoising? > > Yes, that's a good test - that the value propagates and is consistent with the > request. Added tests for both paths, from the capturer's new NeedDenoising method, and setting it via constraints. (No tests for which method takes precedence, though. I think that's ok, applications should use one or the other). > A lower priority item-that-needs-doing is to be able to read back the value of > "denoising" - whether it's off or on. This is needed for getSettings(). You can get the configured value from the VideoTrackSource. But I'm afraid there might be no way to query what the encoder actually is doing.
lgtm, please add the precedence test (constraints apply on top of track) https://codereview.webrtc.org/2334683002/diff/20001/webrtc/api/videocapturert... File webrtc/api/videocapturertracksource_unittest.cc (right): https://codereview.webrtc.org/2334683002/diff/20001/webrtc/api/videocapturert... webrtc/api/videocapturertracksource_unittest.cc:452: TEST_F(VideoCapturerTrackSourceTest, DenoisingConstraintOn) { I would like a test for constraints overriding the capturer/track option as well, I believe the constraints should go on top of capturer settings, and it's OK that they override track defaults. https://codereview.webrtc.org/2334683002/diff/20001/webrtc/media/base/fakevid... File webrtc/media/base/fakevideocapturer.h (right): https://codereview.webrtc.org/2334683002/diff/20001/webrtc/media/base/fakevid... webrtc/media/base/fakevideocapturer.h:126: virtual cricket::CaptureState Start( remove virtual (e.g. only use override and not virtual + override), here and below https://codereview.webrtc.org/2334683002/diff/20001/webrtc/media/base/fakevid... webrtc/media/base/fakevideocapturer.h:158: void SetNeedsDenoising(bool needs_denoising) { Nit: Imo this should take a rtc::Optional<bool> as argument, it makes it more obvious in the tests that the needs_denoising argument is actually tri-state (and makes it posssible to set an unset object).
https://codereview.webrtc.org/2334683002/diff/20001/webrtc/api/videocapturert... File webrtc/api/videocapturertracksource_unittest.cc (right): https://codereview.webrtc.org/2334683002/diff/20001/webrtc/api/videocapturert... webrtc/api/videocapturertracksource_unittest.cc:452: TEST_F(VideoCapturerTrackSourceTest, DenoisingConstraintOn) { On 2016/09/13 14:02:09, pbos-webrtc wrote: > I would like a test for constraints overriding the capturer/track option as > well, I believe the constraints should go on top of capturer settings, and it's > OK that they override track defaults. Done. https://codereview.webrtc.org/2334683002/diff/20001/webrtc/media/base/fakevid... File webrtc/media/base/fakevideocapturer.h (right): https://codereview.webrtc.org/2334683002/diff/20001/webrtc/media/base/fakevid... webrtc/media/base/fakevideocapturer.h:126: virtual cricket::CaptureState Start( On 2016/09/13 14:02:09, pbos-webrtc wrote: > remove virtual (e.g. only use override and not virtual + override), here and > below Done, all occurances in this class. https://codereview.webrtc.org/2334683002/diff/20001/webrtc/media/base/fakevid... webrtc/media/base/fakevideocapturer.h:158: void SetNeedsDenoising(bool needs_denoising) { On 2016/09/13 14:02:09, pbos-webrtc wrote: > Nit: Imo this should take a rtc::Optional<bool> as argument, it makes it more > obvious in the tests that the needs_denoising argument is actually tri-state > (and makes it posssible to set an unset object). Done.
lgtm
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2334683002/#ps40001 (title: "Address pbos' comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8340)
nisse@webrtc.org changed reviewers: + perkj@webrtc.org
Per: Owner's approval needed.
https://codereview.webrtc.org/2334683002/diff/40001/webrtc/api/videocapturert... File webrtc/api/videocapturertracksource.cc (right): https://codereview.webrtc.org/2334683002/diff/40001/webrtc/api/videocapturert... webrtc/api/videocapturertracksource.cc:324: // Note that Chrome no longer uses the webrtc constraints interface. Please remove the comment about chrome. https://codereview.webrtc.org/2334683002/diff/40001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2334683002/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:178: // Returns denoising preference. Unset means use encoder-specific default. This should not have anything todo with an encoder. So I think this comment should not mention an encoder. https://codereview.webrtc.org/2334683002/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:179: virtual rtc::Optional<bool> NeedsDenoising() const { Also - I think this api should be identical to IsScreenCast. Its the same hack. virtual bool NeedsDenoising() { return false; }
https://codereview.webrtc.org/2334683002/diff/40001/webrtc/api/videocapturert... File webrtc/api/videocapturertracksource.cc (right): https://codereview.webrtc.org/2334683002/diff/40001/webrtc/api/videocapturert... webrtc/api/videocapturertracksource.cc:324: // Note that Chrome no longer uses the webrtc constraints interface. On 2016/09/14 13:08:27, perkj_webrtc wrote: > Please remove the comment about chrome. Done. There are other comments in the code explaining Chrome implications, what's the rule? https://codereview.webrtc.org/2334683002/diff/40001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2334683002/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:178: // Returns denoising preference. Unset means use encoder-specific default. On 2016/09/14 13:08:27, perkj_webrtc wrote: > This should not have anything todo with an encoder. So I think this comment > should not mention an encoder. Behavior if unset depends on the encoder. I think that's relevant. https://codereview.webrtc.org/2334683002/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:179: virtual rtc::Optional<bool> NeedsDenoising() const { On 2016/09/14 13:08:27, perkj_webrtc wrote: > Also - I think this api should be identical to IsScreenCast. > Its the same hack. > virtual bool NeedsDenoising() { return false; } I'm afraid it has to be three-state, it's passed on to be returned by VideoCapturerTrackSource::needs_denoising, then copied into VideoOptions for SetVideoSend by VideoRtpSender. For consistency, we could change IsScreencast to the three-state too, but I don't think that is a real improvement.
I see. A not so happy lgtm in that case if you clarify the comments in cricket::VideoCapturer. https://codereview.webrtc.org/2334683002/diff/40001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2334683002/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:178: // Returns denoising preference. Unset means use encoder-specific default. On 2016/09/14 13:20:23, nisse-webrtc wrote: > On 2016/09/14 13:08:27, perkj_webrtc wrote: > > This should not have anything todo with an encoder. So I think this comment > > should not mention an encoder. > > Behavior if unset depends on the encoder. I think that's relevant. > Can you elaborate on this. To me- the whole thing is very weird and hacky. Is there a better comment in VideoTrackSource? Maybe also add a todo and a comment why this is added at all here. https://codereview.webrtc.org/2334683002/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:179: virtual rtc::Optional<bool> NeedsDenoising() const { On 2016/09/14 13:20:23, nisse-webrtc wrote: > On 2016/09/14 13:08:27, perkj_webrtc wrote: > > Also - I think this api should be identical to IsScreenCast. > > Its the same hack. > > virtual bool NeedsDenoising() { return false; } > > I'm afraid it has to be three-state, it's passed on to be returned by > VideoCapturerTrackSource::needs_denoising, then copied into VideoOptions for > SetVideoSend by VideoRtpSender. > > For consistency, we could change IsScreencast to the three-state too, but I > don't think that is a real improvement. Acknowledged.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org, pbos@webrtc.org, perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/2334683002/#ps80001 (title: "TODO comment explaining the NeedsDenoising hack.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by pbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add method cricket::VideoCapturer::NeedsDenoising, use in VideoCapturerTrackSource. BUG=chromium:645907 ========== to ========== Add method cricket::VideoCapturer::NeedsDenoising, use in VideoCapturerTrackSource. BUG=chromium:645907 Committed: https://crrev.com/0d14c6abba19295725519ce38105688ae0a62513 Cr-Commit-Position: refs/heads/master@{#14219} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0d14c6abba19295725519ce38105688ae0a62513 Cr-Commit-Position: refs/heads/master@{#14219}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.webrtc.org/2433293003/ by nisse@webrtc.org. The reason for reverting is: 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.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
