Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1850063003:
Dynamic error tracking in RTCVideoDecoder (Closed)
|
Created:
4 years, 8 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDynamic error tracking in RTCVideoDecoder
This CL dynamically resets the error counter, i.e. if we
start decoding frames successfully, reset the counter so
that periodic failures can be handled as well.
BUG=594161
TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals.
Committed: https://crrev.com/e3de5b456a41997ef6026b06bf70801277a58df9
Cr-Commit-Position: refs/heads/master@{#385544}
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Messages
Total messages: 18 (8 generated)
Dyanmic error tracking in RTCVideoDecoder
Description was changed from ========== dynamics. BUG=594161 ========== to ========== Dynamic error tracking in RTCVideoDecoder BUG=594161 ==========
Description was changed from ========== Dynamic error tracking in RTCVideoDecoder BUG=594161 ========== to ========== Dynamic error tracking in RTCVideoDecoder This CL dynamically resets the error counter, i.e. if we start decoding frames successfully, reset the counter so that periodic failures can be handled as well. BUG=594161 TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals. ==========
emircan@chromium.org changed reviewers: + mcasas@chromium.org
PTAL.
posciak@chromium.org changed reviewers: + wuchengli@chromium.org
looking good - how do you inject the error frames exactly (see the TEST= line in the description). Also could we add some (unit)tests for this functionality? https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.cc:166: // Try reseting the session |kNumVDAErrorsHandled| times. nit: s/the session/ the session up to/ https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.cc:843: void RTCVideoDecoder::IncrementVDAErrorCounter() { This file adds a suffix "_Lock" to the methods that are supposed to work under |lock.AssserAcquired()|, consider doing the same here for consistency. (I'd rather see them suffixed "ThreadSafe" but that's another CL - actually, can you make a bug for that?) https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.cc:854: if (vda_error_counter_ > 0) { if (vda_error_counter_ == 0) return; //... https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_decoder.h (right): https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.h:197: // Keep track of VDA errors. s/Keep/Keeps/ , from [1] "These comments should be descriptive ("Opens the file") rather than imperative ("Open the file"); the comment describes the function, it does not tell the function what to do." Same elsewhere. [1] https://google.github.io/styleguide/cppguide.html#Function_Comments https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.h:258: // |next_bitstream_buffer_id_| and |reset_bitstream_buffer_id_|. Now |lock_| also protects |vda_error_counter_| and |num_consecutive_success_since_error_| correct?
Patchset #2 (id:20001) has been deleted
PTAL again. After extensive testing on CrOS(samus), we have seen that errors might happen up to 15 times in a row waiting for SPS. Therefore, I changed the constants. Also, seeing that the error happens mostly consecutively, I decided to reset the error counter after a single successful decode. https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.cc:166: // Try reseting the session |kNumVDAErrorsHandled| times. On 2016/04/01 16:39:09, mcasas wrote: > nit: s/the session/ the session up to/ Done. https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.cc:843: void RTCVideoDecoder::IncrementVDAErrorCounter() { On 2016/04/01 16:39:09, mcasas wrote: > This file adds a suffix "_Lock" to the > methods that are supposed to work > under |lock.AssserAcquired()|, consider > doing the same here for consistency. > (I'd rather see them suffixed > "ThreadSafe" but that's another CL - > actually, can you make a bug for that?) Done. https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.cc:854: if (vda_error_counter_ > 0) { On 2016/04/01 16:39:09, mcasas wrote: > if (vda_error_counter_ == 0) > return; > //... Done. https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_decoder.h (right): https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.h:197: // Keep track of VDA errors. On 2016/04/01 16:39:09, mcasas wrote: > s/Keep/Keeps/ , from [1] > "These comments should be descriptive ("Opens the file") > rather than imperative ("Open the file"); the comment > describes the function, it does not tell the function what to do." > > Same elsewhere. > > [1] https://google.github.io/styleguide/cppguide.html#Function_Comments Done. https://codereview.chromium.org/1850063003/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_decoder.h:258: // |next_bitstream_buffer_id_| and |reset_bitstream_buffer_id_|. On 2016/04/01 16:39:09, mcasas wrote: > Now |lock_| also protects |vda_error_counter_| and > |num_consecutive_success_since_error_| correct? Yes. Done.
Whats |SPS| in your last PTAL-request? https://codereview.chromium.org/1850063003/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1850063003/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:251: TryResetVDAErrorCounter_Locked(); If |need_to_reset_for_midstream_resize| in l.246, then l.247 would unlock |lock_| and this line would hit an assert, right? https://codereview.chromium.org/1850063003/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.h (right): https://codereview.chromium.org/1850063003/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.h:129: int GetVDAErrorCounter() { return vda_error_counter_; } GetVDAErrorCounterForTesting() ? (The suffix ForTesting will remove it automatically from release-non-test builds).
|SPS| stands for Sequence Parameter Set and acts like the metadata for a sequence of frames. We hit errors in below code ~15 in testing some WebRTC client's application. https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/h264... https://codereview.chromium.org/1850063003/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/1850063003/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.cc:251: TryResetVDAErrorCounter_Locked(); On 2016/04/05 21:32:01, mcasas wrote: > If |need_to_reset_for_midstream_resize| in l.246, > then l.247 would unlock |lock_| and this line would > hit an assert, right? base::AutoUnlock would acquire the lock again when going out of scope on l.249. https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati... https://codereview.chromium.org/1850063003/diff/40001/content/renderer/media/... File content/renderer/media/rtc_video_decoder.h (right): https://codereview.chromium.org/1850063003/diff/40001/content/renderer/media/... content/renderer/media/rtc_video_decoder.h:129: int GetVDAErrorCounter() { return vda_error_counter_; } On 2016/04/05 21:32:01, mcasas wrote: > GetVDAErrorCounterForTesting() ? > (The suffix ForTesting will remove it > automatically from release-non-test > builds). Done.
LGTM my parts
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1850063003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1850063003/60001
Message was sent while issue was closed.
Description was changed from ========== Dynamic error tracking in RTCVideoDecoder This CL dynamically resets the error counter, i.e. if we start decoding frames successfully, reset the counter so that periodic failures can be handled as well. BUG=594161 TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals. ========== to ========== Dynamic error tracking in RTCVideoDecoder This CL dynamically resets the error counter, i.e. if we start decoding frames successfully, reset the counter so that periodic failures can be handled as well. BUG=594161 TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Dynamic error tracking in RTCVideoDecoder This CL dynamically resets the error counter, i.e. if we start decoding frames successfully, reset the counter so that periodic failures can be handled as well. BUG=594161 TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals. ========== to ========== Dynamic error tracking in RTCVideoDecoder This CL dynamically resets the error counter, i.e. if we start decoding frames successfully, reset the counter so that periodic failures can be handled as well. BUG=594161 TEST=Tested on Mac using VT HW decoder, injecting error frames in intervals. Committed: https://crrev.com/e3de5b456a41997ef6026b06bf70801277a58df9 Cr-Commit-Position: refs/heads/master@{#385544} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e3de5b456a41997ef6026b06bf70801277a58df9 Cr-Commit-Position: refs/heads/master@{#385544} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
