Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2988963002:
Add support for a forced software encoder fallback. (Closed)
|
Created:
3 years, 4 months ago by åsapersson Modified:
3 years, 4 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, the sun, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd support for a forced software encoder fallback.
Make it possible to switch from VP8 HW -> VP8 SW -> VP8 HW depending on bitrate and resolution.
BUG=webrtc:6634
Review-Url: https://codereview.webrtc.org/2988963002
Cr-Commit-Position: refs/heads/master@{#19362}
Committed: https://chromium.googlesource.com/external/webrtc/+/22c76c4e65679edabfc99d050d40c944cd5ce092
Patch Set 1 #Patch Set 2 #Patch Set 3 : add max pixel limit #
Total comments: 36
Patch Set 4 : address comments #
Total comments: 6
Patch Set 5 : address comments #Patch Set 6 : fix comment #
Total comments: 3
Patch Set 7 : address comments #
Messages
Total messages: 38 (26 generated)
Patchset #2 (id:20001) has been deleted
Patchset #8 (id:160001) has been deleted
Description was changed from ========== Forced SW fallback... BUG= ========== to ========== Add support for a forced software encoder fallback. Make it possible to switch from VP8 HW -> VP8 SW -> VP8 HW depending on bitrate and resolution. BUG=webrtc:6634 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
Patchset #3 (id:220001) has been deleted
asapersson@webrtc.org changed reviewers: + brandtr@webrtc.org, magjed@webrtc.org, sprang@webrtc.org
Looks good! Added some comments. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:33: VideoEncoderSoftwareFallbackWrapper::ForcedFallbackParams* params) { RTC_DCHECK(params) ? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:70: if (forced_fallback_enabled_) I'd add {} here, to make it a bit more readable. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:178: (ret == WEBRTC_VIDEO_CODEC_OK && StartForcedFallback())) && These conditions are a bit complicated. Could it be expanded into several lines, to make it more readable? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:282: !forced_fallback_.start_ms) { Why do we need to check start_ms here? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.h (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:55: rtc::Optional<int64_t> start_ms; IIUC, start_ms is the timestamp when we go below |low_kbps| ? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:59: const int max_pixels = 76800; // 320 * 240 Maybe remove comment and write max_pixels = 320 * 240 ? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:93: bool forced_fallback_enabled_; How about |forced_fallback_possible_|? This member only shows if we can do the forced fallback, not if we have actually done it. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc:335: &codec_, kNumCores, kMaxPayloadSize)); git cl format? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc:392: TEST_F(ForcedFallbackTestDisabled, NoFallbackWithoutFieldTrial) { Nice unit tests! They answered several questions (now deleted), that I had when reviewing the main code. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc:484: const int numRuns = 5; kNumRuns https://codereview.webrtc.org/2988963002/diff/240001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:254: // SimulcastRateAllocator::OnTemporalLayersCreated but not for VP8 HW. Should we file a bug to track this issue?
https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:44: return; Maybe log a warning if parsing fails? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:171: return WEBRTC_VIDEO_CODEC_ERROR; I know this code isn't new, but I'm not entirely sure I understand what it does. Could you add a comment? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:236: bool VideoEncoderSoftwareFallbackWrapper::StartForcedFallback() { Maybe you can make this something liek a ShouldUseFallback() const method, and change the state outside? Would be easier to reason about this method then. It could also be re-used in TryRelease and TryReinit https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.h (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:52: // for more than |min_low_ms|. If the bitrate is above |high_kbps|, the forced Does same time limit apply for high_kbps? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc:38: VideoEncoderSoftwareFallbackWrapperTest(const std::string& field_trials) explicit https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc:104: bool supports_native_handle_ = false; Should these members be protected? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:254: // SimulcastRateAllocator::OnTemporalLayersCreated but not for VP8 HW. On 2017/08/04 11:16:24, brandtr wrote: > Should we file a bug to track this issue? Why did explicitly de-registering the temporal layers instance in the encoder destructor not work?
Patchset #5 (id:280001) has been deleted
Patchset #4 (id:260001) has been deleted
Patchset #4 (id:300001) has been deleted
Patchset #4 (id:320001) has been deleted
https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:33: VideoEncoderSoftwareFallbackWrapper::ForcedFallbackParams* params) { On 2017/08/04 11:16:23, brandtr wrote: > RTC_DCHECK(params) ? Done. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:44: return; On 2017/08/07 08:59:12, sprang_webrtc wrote: > Maybe log a warning if parsing fails? Done. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:70: if (forced_fallback_enabled_) On 2017/08/04 11:16:23, brandtr wrote: > I'd add {} here, to make it a bit more readable. Done. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:171: return WEBRTC_VIDEO_CODEC_ERROR; On 2017/08/07 08:59:12, sprang_webrtc wrote: > I know this code isn't new, but I'm not entirely sure I understand what it does. > Could you add a comment? Done. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:178: (ret == WEBRTC_VIDEO_CODEC_OK && StartForcedFallback())) && On 2017/08/04 11:16:23, brandtr wrote: > These conditions are a bit complicated. Could it be expanded into several lines, > to make it more readable? Done. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:236: bool VideoEncoderSoftwareFallbackWrapper::StartForcedFallback() { On 2017/08/07 08:59:12, sprang_webrtc wrote: > Maybe you can make this something liek a ShouldUseFallback() const method, and > change the state outside? Would be easier to reason about this method then. It > could also be re-used in TryRelease and TryReinit Changed the code a bit, ptal. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:282: !forced_fallback_.start_ms) { On 2017/08/04 11:16:23, brandtr wrote: > Why do we need to check start_ms here? Checks if it is the forced fallback that is in use. Added method IsForcedFallbackActive. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.h (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:52: // for more than |min_low_ms|. If the bitrate is above |high_kbps|, the forced On 2017/08/07 08:59:12, sprang_webrtc wrote: > Does same time limit apply for high_kbps? No, updated comment. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:55: rtc::Optional<int64_t> start_ms; On 2017/08/04 11:16:23, brandtr wrote: > IIUC, start_ms is the timestamp when we go below |low_kbps| ? Added comment. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:59: const int max_pixels = 76800; // 320 * 240 On 2017/08/04 11:16:23, brandtr wrote: > Maybe remove comment and write max_pixels = 320 * 240 ? Done. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:93: bool forced_fallback_enabled_; On 2017/08/04 11:16:23, brandtr wrote: > How about |forced_fallback_possible_|? This member only shows if we can do the > forced fallback, not if we have actually done it. Done. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc:38: VideoEncoderSoftwareFallbackWrapperTest(const std::string& field_trials) On 2017/08/07 08:59:12, sprang_webrtc wrote: > explicit Done. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc:104: bool supports_native_handle_ = false; On 2017/08/07 08:59:12, sprang_webrtc wrote: > Should these members be protected? I think public is ok? https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc:335: &codec_, kNumCores, kMaxPayloadSize)); On 2017/08/04 11:16:23, brandtr wrote: > git cl format have run it https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc:484: const int numRuns = 5; On 2017/08/04 11:16:23, brandtr wrote: > kNumRuns Done. https://codereview.webrtc.org/2988963002/diff/240001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:254: // SimulcastRateAllocator::OnTemporalLayersCreated but not for VP8 HW. On 2017/08/07 08:59:12, sprang_webrtc wrote: > On 2017/08/04 11:16:24, brandtr wrote: > > Should we file a bug to track this issue? > > Why did explicitly de-registering the temporal layers instance in the encoder > destructor not work? Using something like tl_factory->Remove is not possible from the encoder since the SimulcastRateAllocator might be destructed by then and the TemporalLayerFactory (owned by SimulcastRateAllocator).
lgtm https://codereview.webrtc.org/2988963002/diff/240001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:254: // SimulcastRateAllocator::OnTemporalLayersCreated but not for VP8 HW. On 2017/08/08 10:40:54, åsapersson wrote: > On 2017/08/07 08:59:12, sprang_webrtc wrote: > > On 2017/08/04 11:16:24, brandtr wrote: > > > Should we file a bug to track this issue? > > > > Why did explicitly de-registering the temporal layers instance in the encoder > > destructor not work? > > Using something like tl_factory->Remove is not possible from the encoder since > the SimulcastRateAllocator might be destructed by then and the > TemporalLayerFactory (owned by SimulcastRateAllocator). Ah, that's problematic. This should be fixed, but let's do that in a separate cl. https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc (right): https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:337: return (rtc::TimeMillis() - *start_ms) >= min_low_ms; nit: call rtc::TimeMillis() just once and reuse the value
lgtm https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.h (right): https://codereview.webrtc.org/2988963002/diff/240001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:55: rtc::Optional<int64_t> start_ms; On 2017/08/08 10:40:54, åsapersson wrote: > On 2017/08/04 11:16:23, brandtr wrote: > > IIUC, start_ms is the timestamp when we go below |low_kbps| ? > > Added comment. Thanks. It's easier to understand now. https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc (right): https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:328: uint32_t kbps, bitrate_kbps? https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.h (right): https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:60: bool Start(uint32_t kbps, const VideoCodec& codec); How about naming these ShouldStart and ShouldStop ?
https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc (right): https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:328: uint32_t kbps, On 2017/08/09 09:05:02, brandtr wrote: > bitrate_kbps? Done. https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:337: return (rtc::TimeMillis() - *start_ms) >= min_low_ms; On 2017/08/09 08:38:07, sprang_webrtc wrote: > nit: call rtc::TimeMillis() just once and reuse the value Done. https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.h (right): https://codereview.webrtc.org/2988963002/diff/340001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:60: bool Start(uint32_t kbps, const VideoCodec& codec); On 2017/08/09 09:05:03, brandtr wrote: > How about naming these ShouldStart and ShouldStop ? Done.
Patchset #6 (id:380001) has been deleted
asapersson@webrtc.org changed reviewers: + mflodman@webrtc.org
mflodman: can you please have a look?
One Q and one optional comment, then LGTM. https://codereview.webrtc.org/2988963002/diff/400001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc (right): https://codereview.webrtc.org/2988963002/diff/400001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:199: if (fallback_requested && InitFallbackEncoder()) { Is there a risk this will spam the log-file? I.e. we'll be trying for each incoming frame and log one warning per frame? https://codereview.webrtc.org/2988963002/diff/400001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.h (right): https://codereview.webrtc.org/2988963002/diff/400001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:61: bool ShouldStop(uint32_t bitrate_kbps) const { My preference, so feel free to ignore. I prefer to have all implementations in the cc-file rather than mixing. But that is my preference, so feel free to keep them here if you'd like.
https://codereview.webrtc.org/2988963002/diff/400001/webrtc/media/engine/vide... File webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc (right): https://codereview.webrtc.org/2988963002/diff/400001/webrtc/media/engine/vide... webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc:199: if (fallback_requested && InitFallbackEncoder()) { On 2017/08/15 08:00:04, mflodman wrote: > Is there a risk this will spam the log-file? I.e. we'll be trying for each > incoming frame and log one warning per frame? Yes, if InitFallbackEncoder fails. For the forced fallback case, I updated start_ms to the current time when a fallback is requested to not get too frequent requests (each min_low_ms interval).
The CQ bit was checked by asapersson@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by asapersson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org, sprang@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2988963002/#ps420001 (title: "address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1502869862774380,
"parent_rev": "2b05ba1874cd71ed36c5a5b7094ed1d6018db23f", "commit_rev":
"22c76c4e65679edabfc99d050d40c944cd5ce092"}
Message was sent while issue was closed.
Description was changed from ========== Add support for a forced software encoder fallback. Make it possible to switch from VP8 HW -> VP8 SW -> VP8 HW depending on bitrate and resolution. BUG=webrtc:6634 ========== to ========== Add support for a forced software encoder fallback. Make it possible to switch from VP8 HW -> VP8 SW -> VP8 HW depending on bitrate and resolution. BUG=webrtc:6634 Review-Url: https://codereview.webrtc.org/2988963002 Cr-Commit-Position: refs/heads/master@{#19362} Committed: https://chromium.googlesource.com/external/webrtc/+/22c76c4e65679edabfc99d050... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:420001) as https://chromium.googlesource.com/external/webrtc/+/22c76c4e65679edabfc99d050... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
