Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1328863002:
Allow encoders to fall back dynamically to software. (Closed)
|
Created:
5 years, 3 months ago by noahric Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, andresp, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAllow encoders to fall back dynamically to software.
Like video_decoder.cc, a call to Encode that returns
WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE will trigger an attempted fallback
to a built-in software encoder. Initialization information, along with
any rate and channel parameter info, will be replayed on the software
encoder and then the frame (that cause the fallback) will be immediately
replayed for the software encoder.
Also modified the existing behavior to Release() the "real" encoder even
if a fallback encoder exists. That seems like the correct behavior.
BUG=webrtc:2920
Committed: https://crrev.com/b1ce663d16e0aa882aa4767ea8404f8c48a7370d
Cr-Commit-Position: refs/heads/master@{#10368}
Patch Set 1 #Patch Set 2 : git cl format #
Total comments: 4
Patch Set 3 : CL comments around Release()ing the non-fallback encoder #
Total comments: 6
Patch Set 4 : unit test CR fixes #Patch Set 5 : Rebased #
Messages
Total messages: 25 (9 generated)
noahric@chromium.org changed reviewers: + pbos@webrtc.org
Hey Peter, let me know what you think about this. It'd be handy to allow hardware encode to fallback in the same way as software decode. It's not as recoverable as the decoder, in that it'll only try to recover on InitEncode, but it'll allow for more dynamic failures in the hardware encoder to be recovered from. On Android, at least, that can be helpful for cases where the failure isn't detectable at InitEncode time and isn't recoverable just be recreating the MediaCodec.
overall LG, (I assume that you actually want this for an implementation) https://codereview.webrtc.org/1328863002/diff/20001/webrtc/video/video_encode... File webrtc/video/video_encoder.cc (right): https://codereview.webrtc.org/1328863002/diff/20001/webrtc/video/video_encode... webrtc/video/video_encoder.cc:119: int32_t ret = encoder_->Release(); Either encoder_ or fallback_encoder_ should be initialized, so we shouldn't release both ever? I don't think we should call Release without a corresponding OK InitEncode call and I don't think (?) they should ever be initialized at the same time.
https://codereview.webrtc.org/1328863002/diff/20001/webrtc/video/video_encode... File webrtc/video/video_encoder.cc (right): https://codereview.webrtc.org/1328863002/diff/20001/webrtc/video/video_encode... webrtc/video/video_encoder.cc:59: bool VideoEncoderSoftwareFallbackWrapper::InitFallbackEncoder() { Should this Release() an initialized encoder_ ?
Sorry I let this sit so long, go busy with some other stuff :) PTAL. https://codereview.webrtc.org/1328863002/diff/20001/webrtc/video/video_encode... File webrtc/video/video_encoder.cc (right): https://codereview.webrtc.org/1328863002/diff/20001/webrtc/video/video_encode... webrtc/video/video_encoder.cc:59: bool VideoEncoderSoftwareFallbackWrapper::InitFallbackEncoder() { On 2015/09/09 14:31:02, pbos-webrtc wrote: > Should this Release() an initialized encoder_ ? Done. Also added Release() calls to fallback_encoder_ reset paths, in case implementations require/expect that. https://codereview.webrtc.org/1328863002/diff/20001/webrtc/video/video_encode... webrtc/video/video_encoder.cc:119: int32_t ret = encoder_->Release(); On 2015/09/09 14:30:24, pbos-webrtc wrote: > Either encoder_ or fallback_encoder_ should be initialized, so we shouldn't > release both ever? I don't think we should call Release without a corresponding > OK InitEncode call and I don't think (?) they should ever be initialized at the > same time. I'm worried about two things: 1) encoders not expecting Release->InitEncode. I think that's probably fine. 2) encoders not correctly handling only calls after being Release()ed. Things like SetRates pass values to both. They look correct for e.g. the vp8_impl.cc implementation, but I'm not sure what implementations exist and what they expect. Hopefully they just fail gracefully and the return vals are ignored here, but I'm not sure. Still, done as you requested, so let me know what you think. It's pretty easy to put it back.
https://codereview.webrtc.org/1328863002/diff/40001/webrtc/video/video_encode... File webrtc/video/video_encoder.cc (right): https://codereview.webrtc.org/1328863002/diff/40001/webrtc/video/video_encode... webrtc/video/video_encoder.cc:127: // If the fallback_encoder_ is non-null, it means it was created via I think you can add a test for this one not doing a double release of encoder_. https://codereview.webrtc.org/1328863002/diff/40001/webrtc/video/video_encode... File webrtc/video/video_encoder_unittest.cc (right): https://codereview.webrtc.org/1328863002/diff/40001/webrtc/video/video_encode... webrtc/video/video_encoder_unittest.cc:134: EXPECT_GT(callback_.callback_count_, 0); Not EQ(1, ...) ? or can you fetch it before and expect prev_count + 1? https://codereview.webrtc.org/1328863002/diff/40001/webrtc/video/video_encode... webrtc/video/video_encoder_unittest.cc:152: EXPECT_GT(callback_.callback_count_, 0); EQ(1, callback_.callback_count_) or prev_count + 1?
https://codereview.webrtc.org/1328863002/diff/40001/webrtc/video/video_encode... File webrtc/video/video_encoder.cc (right): https://codereview.webrtc.org/1328863002/diff/40001/webrtc/video/video_encode... webrtc/video/video_encoder.cc:127: // If the fallback_encoder_ is non-null, it means it was created via On 2015/10/20 09:06:56, pbos-webrtc wrote: > I think you can add a test for this one not doing a double release of encoder_. Good point. Added an extra EXPECT_EQ in the InternalEncoderReleasedDuringFallback test to make sure it is released after the UtilizeFallbackEncoder is called (when the InitEncode fails and triggers the fallback) but not an additional time when the overall Release happens. https://codereview.webrtc.org/1328863002/diff/40001/webrtc/video/video_encode... File webrtc/video/video_encoder_unittest.cc (right): https://codereview.webrtc.org/1328863002/diff/40001/webrtc/video/video_encode... webrtc/video/video_encoder_unittest.cc:134: EXPECT_GT(callback_.callback_count_, 0); On 2015/10/20 09:06:56, pbos-webrtc wrote: > Not EQ(1, ...) ? or can you fetch it before and expect prev_count + 1? Done (prev_count + 1). Also updated the encode_count_ check to grab the value before and expect it to match, instead of hardcoding 0. https://codereview.webrtc.org/1328863002/diff/40001/webrtc/video/video_encode... webrtc/video/video_encoder_unittest.cc:152: EXPECT_GT(callback_.callback_count_, 0); On 2015/10/20 09:06:56, pbos-webrtc wrote: > EQ(1, callback_.callback_count_) or prev_count + 1? Done (prev_count + 1 for both).
Description was changed from ========== Allow encoders to fall back dynamically to software. Like video_decoder.cc, a call to Encode that returns WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE will trigger an attempted fallback to a built-in software encoder. Initialization information, along with any rate and channel parameter info, will be replayed on the software encoder and then the frame (that cause the fallback) will be immediately replayed for the software encoder. Also modified the existing behavior to Release() the "real" encoder even if a fallback encoder exists. That seems like the correct behavior. BUG= ========== to ========== Allow encoders to fall back dynamically to software. Like video_decoder.cc, a call to Encode that returns WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE will trigger an attempted fallback to a built-in software encoder. Initialization information, along with any rate and channel parameter info, will be replayed on the software encoder and then the frame (that cause the fallback) will be immediately replayed for the software encoder. Also modified the existing behavior to Release() the "real" encoder even if a fallback encoder exists. That seems like the correct behavior. BUG=webrtc:2920 ==========
lgtm
The CQ bit was checked by pbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328863002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328863002/60001
The CQ bit was unchecked by pbos@webrtc.org
The CQ bit was checked by noahric@chromium.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/1328863002/#ps80001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328863002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328863002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1354)
noahric@chromium.org changed reviewers: + mflodman@webrtc.org
+magnus for toplevel owners for video_encoder.h. Magnus/Peter: does it make sense to either move these headers into a subdirectory or give pbos per-file owners for these headers? Or is it desirable to have to get toplevel webrtc owners for these changes? I've been bit by it twice now, which is also fixable by me actually checking ahead of time (sorry!), but it's somewhat painful. Or maybe this is a feature request for the chromium code review tool to actually show OWNERS information :)
video_encoder.h LGTM And I'll look into the OWNER files and if we can do a per-file OWNER. Or move to a separate folder like you wrote.
On 2015/10/22 06:46:27, mflodman wrote: > video_encoder.h LGTM > > And I'll look into the OWNER files and if we can do a per-file OWNER. Or move to > a separate folder like you wrote. Awesome, thank you!
The CQ bit was checked by noahric@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328863002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328863002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b1ce663d16e0aa882aa4767ea8404f8c48a7370d Cr-Commit-Position: refs/heads/master@{#10368} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
