Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1817573004:
Handle HW decode failures in RTCVideoDecoder (Closed)
|
Created:
4 years, 9 months ago by emircan Modified:
4 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wuchengli Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle HW decode failures in RTCVideoDecoder
This CL adds functionality to reset underlying HW decoder implementation
after an error happens.
BUG=594161
TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals.
Committed: https://crrev.com/d73415ce0c9e0d6c334612f3c3395b8162093586
Cr-Commit-Position: refs/heads/master@{#384019}
Patch Set 1 #
Total comments: 10
Patch Set 2 : tommi@ comments. #
Total comments: 14
Patch Set 3 : mcasas@ comments. #
Total comments: 10
Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Messages
Total messages: 27 (10 generated)
Description was changed from ========== necessary stuff BUG= ========== to ========== Handle HW decode failures in RTCVideoDecoder This CL adds functionality to reset underlying HW decoder implementation after an error happens. BUG=594161 TEST=Tested on Mac using VT hw decoder. ==========
Description was changed from ========== Handle HW decode failures in RTCVideoDecoder This CL adds functionality to reset underlying HW decoder implementation after an error happens. BUG=594161 TEST=Tested on Mac using VT hw decoder. ========== to ========== Handle HW decode failures in RTCVideoDecoder This CL adds functionality to reset underlying HW decoder implementation after an error happens. BUG=594161 TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals. ==========
emircan@chromium.org changed reviewers: + mcasas@chromium.org, pbos@chromium.org, wuchengli@chromium.org
PTAL.
Drive by comments https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.cc:619: waiter.Wait();; One semicolon. Can we avoid waiting? https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.cc:622: if (vda_) Turn this into an else? https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_decoder_unittest.cc (right): https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder_unittest.cc:269: const gfx::Size kInputFrameSize(16, 16); This isn't a compile time constant (static), so should be named input_frame_size. https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder_unittest.cc:276: rtc_decoder_->Decode(input_image, false, NULL, NULL, 0)); Nit: nullptr https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder_unittest.cc:289: INSTANTIATE_TEST_CASE_P(, Intentionally empty?
Sorry. I don't have time to review this for the next several days. Putting myself in cc.
wuchengli@chromium.org changed reviewers: - wuchengli@chromium.org
https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.cc:619: waiter.Wait();; On 2016/03/19 09:25:49, tommi-chromium wrote: > One semicolon. > Can we avoid waiting? Done. https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.cc:622: if (vda_) On 2016/03/19 09:25:49, tommi-chromium wrote: > Turn this into an else? Done. https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_decoder_unittest.cc (right): https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder_unittest.cc:269: const gfx::Size kInputFrameSize(16, 16); On 2016/03/19 09:25:49, tommi-chromium wrote: > This isn't a compile time constant (static), so should be named > input_frame_size. I will put them as const static vars. https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder_unittest.cc:276: rtc_decoder_->Decode(input_image, false, NULL, NULL, 0)); On 2016/03/19 09:25:49, tommi-chromium wrote: > Nit: nullptr Done, changing here and rest of the class. https://codereview.chromium.org/1817573004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder_unittest.cc:289: INSTANTIATE_TEST_CASE_P(, On 2016/03/19 09:25:50, tommi-chromium wrote: > Intentionally empty? Yes, but I guess it would be better to write out that we are going through codec profiles.
emircan@chromium.org changed reviewers: + tommi@chromium.org
I don't think I'm familiar enough with Chromium threading or the RTCVideoDecoder to review this https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:618: else {}s for if-elses
On 2016/03/22 21:20:43, pbos wrote: > I don't think I'm familiar enough with Chromium threading or the RTCVideoDecoder > to review this > > https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... > File content/renderer/media/rtc_video_decoder.cc (right): > > https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... > content/renderer/media/rtc_video_decoder.cc:618: else > {}s for if-elses Thanks. Pinging for tommi@ and mcasas@ who might have input.
Looking good, some comments. https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:166: FROM_HERE, base::Bind(&RTCVideoDecoder::ResetInternal, Shouldn't we make sure that the |state_ = RESETTING| either a) here b) ResetInternal() so that when several |inputImage|s come back to back, we don't post several ResetInternal()s ? https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:168: return WEBRTC_VIDEO_CODEC_ERROR; Following offline conversation, please add some kind of counter of the times we tried to restart the underlying VDA, and if > say, 5, then stick to replying |WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE|. https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:614: DVLOG(2) << "ResetInternal"; nit: Move this to before the DCHECK and use DVLOG(2) << __FUNCTION__; https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:618: else On 2016/03/22 21:20:43, pbos wrote: > {}s for if-elses Not for both bodies being one-liners. https://google.github.io/styleguide/cppguide.html#Conditionals https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:723: PutSHM_Locked(scoped_ptr<base::SharedMemory>(buffer.second)); nit: totally not your problem and prob beyond this CL scope, but this method should be called PutSHM_ThreadSafe() and not _Locked since the latter reads to me as it is going to acquire a lock, potentially the same as in l.719 and then bam! https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... File content/renderer/media/rtc_video_decoder_unittest.cc (right): https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder_unittest.cc:38: // TODO(wuchengli): add MockSharedMemroy so more functions can be tested. nit: s/MockSharedMemroy/MockSharedMemory/ https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder_unittest.cc:146: media::MockVideoDecodeAccelerator* mock_reset_vda_; s/mock_reset_vda_/mock_vda_after_reset_/ ?
https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:166: FROM_HERE, base::Bind(&RTCVideoDecoder::ResetInternal, On 2016/03/23 18:29:56, mcasas wrote: > Shouldn't we make sure that the > |state_ = RESETTING| either > a) here > b) ResetInternal() > > so that when several |inputImage|s come back > to back, we don't post several ResetInternal()s ? Agreed. I will set it here(a) since all |state_| modifications happen under a lock. https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:168: return WEBRTC_VIDEO_CODEC_ERROR; On 2016/03/23 18:29:56, mcasas wrote: > Following offline conversation, please add > some kind of counter of the times we tried to > restart the underlying VDA, and if > say, 5, > then stick to replying > |WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE|. Done. https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:614: DVLOG(2) << "ResetInternal"; On 2016/03/23 18:29:56, mcasas wrote: > nit: Move this to before the DCHECK > and use DVLOG(2) << __FUNCTION__; Done. https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:723: PutSHM_Locked(scoped_ptr<base::SharedMemory>(buffer.second)); On 2016/03/23 18:29:57, mcasas wrote: > nit: totally not your problem and prob > beyond this CL scope, but this method > should be called > PutSHM_ThreadSafe() and not _Locked > since the latter reads to me as it is > going to acquire a lock, potentially the > same as in l.719 and then bam! I agree the name is confusing. They specify "lock_.AssertAcquired();" in the functions at least, so that we can tell Locked stands for "call this only when you have it locked". This class can defintiely use some refactoring and I will revisit it for sure. https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... File content/renderer/media/rtc_video_decoder_unittest.cc (right): https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder_unittest.cc:38: // TODO(wuchengli): add MockSharedMemroy so more functions can be tested. On 2016/03/23 18:29:57, mcasas wrote: > nit: s/MockSharedMemroy/MockSharedMemory/ Done. https://codereview.chromium.org/1817573004/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_decoder_unittest.cc:146: media::MockVideoDecodeAccelerator* mock_reset_vda_; On 2016/03/23 18:29:57, mcasas wrote: > s/mock_reset_vda_/mock_vda_after_reset_/ ? Done.
LGTM % nits/comments below -- assuming the bots, which seem to have a legit complaint, are sorted out. https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:31: const uint32_t kNumVDAErrorsHandled = 5; micro-pedantic-nit: s/kNumVDAErrorsHandled/kNumVDAResetsBeforeGivingUp/ ? https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:166: DVLOG(1) << num_vda_errors_ nit: s/DVLOG(1)/DLOG(ERROR)/ https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:490: num_vda_errors_++; nit: prefer pre-increment. I know, I know, we could debate forever on the effect of idioms on actual real performance, but humour me... :) https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:627: } No {} for one-liners as I replied to pbos@ (this is also pervasive around Chrome code) https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.h (right): https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.h:213: // Number of time that |vda_| notified of an error. s/time/times/
Thanks. https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:31: const uint32_t kNumVDAErrorsHandled = 5; On 2016/03/28 18:15:20, mcasas wrote: > micro-pedantic-nit: > s/kNumVDAErrorsHandled/kNumVDAResetsBeforeGivingUp/ ? kNumVDAResetsBeforeSWFallback? https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:166: DVLOG(1) << num_vda_errors_ On 2016/03/28 18:15:20, mcasas wrote: > nit: s/DVLOG(1)/DLOG(ERROR)/ Done. https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:490: num_vda_errors_++; On 2016/03/28 18:15:20, mcasas wrote: > nit: prefer pre-increment. > I know, I know, we could debate forever > on the effect of idioms on actual real > performance, but humour me... :) Done. https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:627: } On 2016/03/28 18:15:20, mcasas wrote: > No {} for one-liners as I replied to pbos@ > (this is also pervasive around Chrome code) Done. https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.h (right): https://codereview.chromium.org/1817573004/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.h:213: // Number of time that |vda_| notified of an error. On 2016/03/28 18:15:20, mcasas wrote: > s/time/times/ Done.
PING, tommi@ can you PTAL?
hbos@chromium.org changed reviewers: + hbos@chromium.org
Nice, looking forward to seeing if resetting fixes my bug. https://codereview.chromium.org/1817573004/diff/60001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1817573004/diff/60001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:169: return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; What if there is no SW decoder available? Would it continue to use this decoder and get stuck in the DECODE_ERROR state? Also, I don't know what causes decode errors, but if decode errors periodically happen, for whatever reason, we might sooner or later always end up with SW decoder for longer video sessions. In a bug of mine (crbug.com/583704) decode errors happen seemingly randomly on some machines, but not often enough that it wouldn't make sense to recover (if possible).
https://codereview.chromium.org/1817573004/diff/60001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1817573004/diff/60001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:169: return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; On 2016/03/29 12:28:19, hbos_chromium wrote: > What if there is no SW decoder available? Would it continue to use this decoder > and get stuck in the DECODE_ERROR state? > > Also, I don't know what causes decode errors, but if decode errors periodically > happen, for whatever reason, we might sooner or later always end up with SW > decoder for longer video sessions. > In a bug of mine (crbug.com/583704) decode errors happen seemingly randomly on > some machines, but not often enough that it wouldn't make sense to recover (if > possible). That logic is handled in the WebRTC side: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... AFAIU, it will try to use a SW encoder if there is and stop things with an error message otherwise. If HW errors are happening periodically, maybe we can increase the limit of 5. But, I think falling back to SW is a safer bet then?
https://codereview.chromium.org/1817573004/diff/60001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1817573004/diff/60001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:169: return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; On 2016/03/29 19:37:55, emircan wrote: > On 2016/03/29 12:28:19, hbos_chromium wrote: > > What if there is no SW decoder available? Would it continue to use this > decoder > > and get stuck in the DECODE_ERROR state? > > > > Also, I don't know what causes decode errors, but if decode errors > periodically > > happen, for whatever reason, we might sooner or later always end up with SW > > decoder for longer video sessions. > > In a bug of mine (crbug.com/583704) decode errors happen seemingly randomly on > > some machines, but not often enough that it wouldn't make sense to recover (if > > possible). > > That logic is handled in the WebRTC side: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > AFAIU, it will try to use a SW encoder if there is and stop things with an error > message otherwise. > > If HW errors are happening periodically, maybe we can increase the limit of 5. > But, I think falling back to SW is a safer bet then? Yeah, makes sense. Let's not assume HW errors occur periodically. Hopefully there is a root cause that can be addressed. I'm still debugging my bug. I wanted to try this out today in combination with my SW decoder but I've been unable to reliably reproduce the decode error on my mac :/ I'll try again tomorrow on my windows laptop.
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1817573004/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1817573004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1817573004/80001
Message was sent while issue was closed.
Description was changed from ========== Handle HW decode failures in RTCVideoDecoder This CL adds functionality to reset underlying HW decoder implementation after an error happens. BUG=594161 TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals. ========== to ========== Handle HW decode failures in RTCVideoDecoder This CL adds functionality to reset underlying HW decoder implementation after an error happens. BUG=594161 TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Handle HW decode failures in RTCVideoDecoder This CL adds functionality to reset underlying HW decoder implementation after an error happens. BUG=594161 TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals. ========== to ========== Handle HW decode failures in RTCVideoDecoder This CL adds functionality to reset underlying HW decoder implementation after an error happens. BUG=594161 TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals. Committed: https://crrev.com/d73415ce0c9e0d6c334612f3c3395b8162093586 Cr-Commit-Position: refs/heads/master@{#384019} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d73415ce0c9e0d6c334612f3c3395b8162093586 Cr-Commit-Position: refs/heads/master@{#384019} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
