Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
791573002:
fix video_decode_accelerator_unittest issue when decode speed is slow (Closed)
|
Created:
6 years ago by halley.zhao Modified:
4 years, 11 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionfix video_decode_accelerator_unittest issue when decode speed is slow
when decode speed is slow, a BitstreamBuffer is released after the EOS-DecodeNextFragment(),
trigger DecodeNextFragment again in NotifyEndOfBitstreamBuffer (it will flush decode).
BUG=439831
Patch Set 1 : please Ignore it #Patch Set 2 : please ignore it #Patch Set 3 : Fix issue in video unit test that Flush/Reset is not reached when decode speed is slow #
Total comments: 2
Patch Set 4 : fix video_decode_accelerator_unittest issue when decode speed is slow #
Total comments: 5
Patch Set 5 : fix video_decode_accelerator_unittest issue when decoding is slow #
Total comments: 1
Patch Set 6 : fix video_decode_accelerator_unittest issue when decoding is slow #
Total comments: 3
Messages
Total messages: 26 (10 generated)
halley.zhao@intel.com changed reviewers: + asvitkine@chromium.org
halley.zhao@intel.com changed reviewers: + posciak@chromium.org
The CQ bit was unchecked by halley.zhao@intel.com
The CQ bit was checked by halley.zhao@intel.com
The CQ bit was checked by halley.zhao@intel.com
The CQ bit was unchecked by halley.zhao@intel.com
https://codereview.chromium.org/791573002/diff/20001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/791573002/diff/20001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:550: // when decode speed is slow, a BitstreamBuffer is released after the Nit: Comments should be full sentences. https://codereview.chromium.org/791573002/diff/20001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:553: || (encoded_data_next_pos_to_decode_ == encoded_data_.size() Nit: When wrapping, operators (e.g. || and &&) should go on the previous line.
Please explain in more detail the problem you are experiencing and submit a crbug.com/new for it, hopefully with reproduction steps. I do not understand the issue and your comments in the code are not sufficient. Thank you.
On 2014/12/10 12:57:07, Pawel Osciak wrote: > Please explain in more detail the problem you are experiencing and submit a > crbug.com/new for it, hopefully with reproduction steps. > I do not understand the issue and your comments in the code are not sufficient. > Thank you. https://code.google.com/p/chromium/issues/detail?id=439831 when decode_calls_per_second_ is not zero, DecodeNextFragment is issued by itself; independent from how many buffers has really been decoded(NotifyEndOfBitstreamBuffer). so, if DecodeNextFragment runs faster than NotifyEndOfBitstreamBuffer; the following condition fail when DecodeNextFragment runs till the last buffer [outstanding_decodes_ == 0 fails]; then decoder get no chance to Flush(). if (encoded_data_next_pos_to_decode_ == encoded_data_.size()) { if (outstanding_decodes_ == 0) { decoder_->Flush(); SetState(CS_FLUSHING); } return; }
On 2014/12/09 14:27:36, Alexei Svitkine wrote: > https://codereview.chromium.org/791573002/diff/20001/content/common/gpu/media... > File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): > > https://codereview.chromium.org/791573002/diff/20001/content/common/gpu/media... > content/common/gpu/media/video_decode_accelerator_unittest.cc:550: // when > decode speed is slow, a BitstreamBuffer is released after the > Nit: Comments should be full sentences. > > https://codereview.chromium.org/791573002/diff/20001/content/common/gpu/media... > content/common/gpu/media/video_decode_accelerator_unittest.cc:553: || > (encoded_data_next_pos_to_decode_ == encoded_data_.size() > Nit: When wrapping, operators (e.g. || and &&) should go on the previous line. update the patch for comments and coding style. thanks.
asvitkine@chromium.org changed reviewers: + ccameron@chromium.org
asvitkine@chromium.org changed reviewers: + ccameron@chromium.org
Removing myself from reviewers and adding ccameron who hopefully knows the code a bit more that's being changed.
asvitkine@chromium.org changed reviewers: - asvitkine@chromium.org
anyone can give some comments?
posciak@chromium.org changed reviewers: + owenlin@chromium.org
https://codereview.chromium.org/791573002/diff/40001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/791573002/diff/40001/AUTHORS#newcode525 AUTHORS:525: Halley Zhao <halley.zhao@intel.com> I think this file is sorted. https://codereview.chromium.org/791573002/diff/40001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/791573002/diff/40001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:550: // when decoding speed is slow, some BitstreamBuffer may be released s/when/When/ s/speed// https://codereview.chromium.org/791573002/diff/40001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:552: // outstanding_decodes_ is not zero at that time. |outstanding_decodes_| https://codereview.chromium.org/791573002/diff/40001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:553: // we need trigger DecodeNextFragment another time to flush decode. In order to flush the decoder, we need trigger DecodeNextFragment when the last BitstreamBuffer released. https://codereview.chromium.org/791573002/diff/40001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:556: DecodeNextFragment(); How could you prevent to flush the decoder twice ? I am thinking maybe we should move the flush decoder part from DecodeNextFragment to NotifyEndOfBitstreamBuffer. How do you think?
> https://codereview.chromium.org/791573002/diff/40001/content/common/gpu/media... > content/common/gpu/media/video_decode_accelerator_unittest.cc:556: > DecodeNextFragment(); > How could you prevent to flush the decoder twice ? after the last BitstreamBuffer is sent to decoder(EOS), outstanding_decodes_ reach zero only once (there is always "--outstanding_decodes_;" in NotifyEndOfBitstreamBuffer) so flush is called once only. > > I am thinking maybe we should move the flush decoder part from > DecodeNextFragment to NotifyEndOfBitstreamBuffer. How do you think? I updated the patch to follow it. thanks
lgtm % a nit Hi Pawel, can you do a owner review of this CL? Thanks. https://codereview.chromium.org/791573002/diff/60001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/791573002/diff/60001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:751: if (encoded_data_next_pos_to_decode_ == encoded_data_.size()) { Remove '{}'.
On 2015/01/14 06:25:51, Owen Lin wrote: > lgtm % a nit > > Hi Pawel, can you do a owner review of this CL? Thanks. > > https://codereview.chromium.org/791573002/diff/60001/content/common/gpu/media... > File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): > > https://codereview.chromium.org/791573002/diff/60001/content/common/gpu/media... > content/common/gpu/media/video_decode_accelerator_unittest.cc:751: if > (encoded_data_next_pos_to_decode_ == encoded_data_.size()) { > Remove '{}'. thanks, updated
https://codereview.chromium.org/791573002/diff/80001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/791573002/diff/80001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:550: // flush decoder after all BitstreamBuffer are processed Comments should be full English sentences when possible. Please capitalize and use a dot at the end of the sentence. https://codereview.chromium.org/791573002/diff/80001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:552: outstanding_decodes_ == 0) { If the issue is with the decode being slow, shouldn't the proper place for flush be after we receive the final decoded picture via PictureReady? Decoders may be returning bitstream buffers before finishing decode for it.
https://codereview.chromium.org/791573002/diff/80001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/791573002/diff/80001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:552: outstanding_decodes_ == 0) { On 2015/01/20 02:18:00, Pawel Osciak wrote: > If the issue is with the decode being slow, shouldn't the proper place for flush > be after we receive the final decoded picture via PictureReady? Decoders may be > returning bitstream buffers before finishing decode for it. The issue happens only when |decode_calls_per_second_| is not 0. In that mode, we will call DecodeNextFragment() at a constant rate (see l.803 of the base code). However, when we try to decode the last fragment. The previous bitstream buffer may still be used by the decoder (This is the slowness). So that |outstanding_decodes_| is not 0 and we will refuse to flush the decoder (see l.746 of the base code). That's the bug. I think it is correct to move Flush() here since we would like to do the Flush() after the decoder processes the last bitstream buffer. Besides, it is hard to tell which one is the "final" picture (I thought the decoder is allowed to keep some reference frames until we call Flush()).
Halley, Could you please address Pawel's comments. We may need to merge this patch soon. So many thanks. |
