Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
239893002:
Allow multiple concurrent Decode() requests in VideoDecoder interface. (Closed)
|
Created:
6 years, 8 months ago by Sergey Ulanov Modified:
6 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAllow multiple concurrent Decode() requests in VideoDecoder interface.
Previosly VideoDecoder interface allowed only one pending Decode()
request. This meant that decoders that want to decode multiple buffers
simultaneously had to delay frames in order get multiple buffers from
demuxer. Now VideoDecode implementation may allow multiple concurrent
Decode() requests, which is determined by the new
VideoDecoder::GetMaxDecodeRequests(). Also updated DecodeStream to
allow decoding multiple frames in parallel.
BUG=338529
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268140
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 28
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 49
Patch Set 6 : #
Total comments: 13
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 6
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : fix windows #Patch Set 12 : #
Messages
Total messages: 34 (0 generated)
I haven't added any new tests for this change yet - I'd like to get some feedback on the approach before I spend too much time on it. Existing tests pass. Please let me know what you think about this solution.
The basic approach looks fine to me, though I believe xhwang@ has strong feelings on this subject, so adding him to R=. https://codereview.chromium.org/239893002/diff/1/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/239893002/diff/1/media/base/video_decoder.h#n... media/base/video_decoder.h:52: // parallel. An alternative approach that may work better is to return true/false depending on whether the decoder was able to accept the request. (better for the client who doesn't have to track the # of outstanding requests?) https://codereview.chromium.org/239893002/diff/1/media/filters/decoder_stream.cc File media/filters/decoder_stream.cc (right): https://codereview.chromium.org/239893002/diff/1/media/filters/decoder_stream... media/filters/decoder_stream.cc:223: static_cast<int>(ready_output_buffers_.size()) + pending_decode_requests_; Why does it matter how many ready_output_buffers_ there are? I suspect what you really want is to cap the # of ready buffers with a separate limit. https://codereview.chromium.org/239893002/diff/1/media/filters/decoder_stream.h File media/filters/decoder_stream.h (right): https://codereview.chromium.org/239893002/diff/1/media/filters/decoder_stream... media/filters/decoder_stream.h:186: // Decoded buffers that haven't been read yet. Used when the decode supports decode -> decoder https://codereview.chromium.org/239893002/diff/1/media/filters/fake_video_dec... File media/filters/fake_video_decoder.cc (right): https://codereview.chromium.org/239893002/diff/1/media/filters/fake_video_dec... media/filters/fake_video_decoder.cc:74: LOG(ERROR) << "Queue " << decoded_frames_.size(); drop? https://codereview.chromium.org/239893002/diff/1/media/filters/fake_video_dec... media/filters/fake_video_decoder.cc:84: LOG(ERROR) << "Queue " << decoded_frames_.size(); drop? https://codereview.chromium.org/239893002/diff/1/media/filters/fake_video_dec... media/filters/fake_video_decoder.cc:120: LOG(ERROR) << "Queue " << decoded_frames_.size(); drop?
https://codereview.chromium.org/239893002/diff/1/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/239893002/diff/1/media/base/video_decoder.h#n... media/base/video_decoder.h:52: // parallel. On 2014/04/16 01:00:00, Ami Fischman wrote: > An alternative approach that may work better is to return true/false depending > on whether the decoder was able to accept the request. > (better for the client who doesn't have to track the # of outstanding requests?) Isn't DecodeStream the only client of this interface? Also I think GetMaxDecodeRequests() would need to be exposed anyway, so that DecodeStream knows how many output buffers it may need to buffer. https://codereview.chromium.org/239893002/diff/1/media/filters/decoder_stream.cc File media/filters/decoder_stream.cc (right): https://codereview.chromium.org/239893002/diff/1/media/filters/decoder_stream... media/filters/decoder_stream.cc:223: static_cast<int>(ready_output_buffers_.size()) + pending_decode_requests_; On 2014/04/16 01:00:00, Ami Fischman wrote: > Why does it matter how many ready_output_buffers_ there are? > I suspect what you really want is to cap the # of ready buffers with a separate > limit. We need to limit number of output buffers waiting in ready_output_buffers_. It doesn't make sense to request another decode request if there are already some buffers in the queue, so we need to take into account size of ready_output_buffers_ when making decision to decode another frame. If Read() is never called again then we will end up with ready_output_buffers_.size() + pending_decode_requests_ total output buffers waiting in the queue. We could have a separate limit on the size of the output queue, but then it shouldn't be lower than GetMaxDecodeRequests() (otherwise we wouldn't always saturate the decoder, depending on when and how Read() is called), and it's never necessary to have a queue longer than GetMaxDecodeRequests(). I don't feel strongly about this logic so I can change it if you think a different approach would work better. https://codereview.chromium.org/239893002/diff/1/media/filters/decoder_stream.h File media/filters/decoder_stream.h (right): https://codereview.chromium.org/239893002/diff/1/media/filters/decoder_stream... media/filters/decoder_stream.h:186: // Decoded buffers that haven't been read yet. Used when the decode supports On 2014/04/16 01:00:00, Ami Fischman wrote: > decode -> decoder Done. https://codereview.chromium.org/239893002/diff/1/media/filters/fake_video_dec... File media/filters/fake_video_decoder.cc (right): https://codereview.chromium.org/239893002/diff/1/media/filters/fake_video_dec... media/filters/fake_video_decoder.cc:74: LOG(ERROR) << "Queue " << decoded_frames_.size(); On 2014/04/16 01:00:00, Ami Fischman wrote: > drop? Done. https://codereview.chromium.org/239893002/diff/1/media/filters/fake_video_dec... media/filters/fake_video_decoder.cc:84: LOG(ERROR) << "Queue " << decoded_frames_.size(); On 2014/04/16 01:00:00, Ami Fischman wrote: > drop? Done. https://codereview.chromium.org/239893002/diff/1/media/filters/fake_video_dec... media/filters/fake_video_decoder.cc:120: LOG(ERROR) << "Queue " << decoded_frames_.size(); On 2014/04/16 01:00:00, Ami Fischman wrote: > drop? Done.
On Tue, Apr 15, 2014 at 6:44 PM, <sergeyu@chromium.org> wrote: > I don't feel strongly about this logic so I can change it if you think a > different approach would work better. > I also don't feel strongly and defer to xhwang. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry for late reply. This CL touches some fundamental principle that we keep for long time (I am not saying it's good!) so I really need to think hard reviewing this. scherkus@: I believe you need to take a quick look as well (about the high level idea). I updated the bug to clarify what the problem is and what we want to achieve. I like many parts of this CL (e.g. the cleanup of GVD), but I think we still need to work out details to have a clean/correct solution. Here are my high level comments: 0) We should have some high level documentation about how things work in DecoderStream. 1) I like the idea of dropping the internal queue in GVD. 2) Previously we use kNotEnoughData as a way to tigger GpuVideoDecoder to do parallel decoding (which is a bit hacky). Now this logic is controlled by DecoderStream, which looks nice. 3) We should modify FakeVideoDecoder so that it can simulate what GVD is doing. Then we can test: a) All decoded frames are available for VideoRendererImpl. b) When the decoding speed is slow, we can do parallel decoding as much as we can (fully doing kMaxInFlightDecodes decodes). See more detailed comments below. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... File media/filters/decoder_stream.cc (right): https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:89: state_ == STATE_ERROR || state_ == STATE_REINITIALIZING_DECODER) |state_| can also be STATE_PENDING_DEMUXER_READ. Imagine: DS::Read() { Read satisfied because ready_output_buffers isn't empty() (line 106). ReadFromDemuxerStream(); // line 123; state_ changed to STATE_PENDING_DEMUXER_READ } // Since previous Read() was satisfied. DS::Read() { // Now state is STATE_PENDING_DEMUXER_READ. } Actually you have the check against STATE_PENDING_DEMUXER_READ on line 122 :) https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:111: if (state_ == STATE_REINITIALIZING_DECODER) Might worth a comment how this could happen. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:222: int buffers_in_queue = In media/ we use "buffer" for compressed data, e.g. DecoderBuffer. In this class, we use "output" to refer to decoded output. Maybe we should rename buffers_in_queue and ready_output_buffers... https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:224: return buffers_in_queue < decoder_->GetMaxDecodeRequests(); Probably add a comment about why we choose to do this. My understanding is: if decoding is slow and DS's buffers_in_queue is running low, then we can trigger to do parallel decoding at the maximum speed. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:330: } Now we could have multiple Decode() calls in flight, but we still only have one |read_cb_|. That's why SatisfyRead() always assumes that the |read_cb_| is not null. So when pending_decode_requests_ > 1 and we call DecoderStream::Reset(), I assume OnDecodeOutputReady() will be called for multiple times before the decoder is fully reset. In that case, we'll call SatisfyRead() multiple times, which won't work. I think what we need to do is only SatisfyRead once. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:351: AbortRead(); ditto. We only need to AbortRead() once. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:375: while ((extra_output = decoder_->GetDecodeOutput()) != NULL) { Now GetDecodeOutput() will always return NULL for GVD. So this function is only used by the audio decoders (where one input can result in multiple outputs). We may need to clean this up a bit in the future. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:456: if (state_ == STATE_NORMAL && CanDecodeAnotherBuffer()) { We set state_ to be STATE_NORMAL on line 410, so here state_ has to be STATE_NORMAL? https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... File media/filters/gpu_video_decoder.cc (left): https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.cc:589: } Kudos for removing this logic! https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.cc:327: return kMaxInFlightDecodes; [not related to this CL] fischman: Do you know what's the decoding delay of VDAs? Are they guaranteed to be < kMaxInFlightDecodes? Up until this CL, we always schedule kMaxInFlightDecodes decodes upon the first DS::Read() call. This seems to work. But if we have a VDA that has decoding delay > kMaxInFlightDecodes, then we won't get any decoded frames after kMaxInFlightDecodes decode calls, and DS will not issue new decode calls, hence a deadlock. https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.cc:476: CHECK(!pending_decode_callbacks_.empty()); This is catching a coding error, use DCHECK? https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.cc:587: DeliverFrame(VideoFrame::CreateEOSFrame()); I suspect that after FlushDone, we'll still have unsatisfied decode_cb's. (See comments above about decoding delay.) https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... File media/filters/gpu_video_decoder.h (left): https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.h:186: std::list<scoped_refptr<VideoFrame> > ready_video_frames_; Drop this? https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... File media/filters/gpu_video_decoder.h (right): https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.h:139: std::list<DecodeCB> pending_decode_callbacks_; Usually decoders have decoding delay, e.g. for a decoding delay of 3: input 0 1 2 3 4 5 6 ... output 0 1 2 3 ... Assuming kMaxInFlightDecodes > 3 (see my question to fischman@ below), it seems we'll always have at least 3 pending decode_cb in pending_decode_callbacks_. The result of this is that the effective max number of in-flight decodes are dropped to kMaxInFlightDecodes - 3. This will be bad if decoding is slow and we really want to have kMaxInFlightDecodes decodes in parallel.
https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... File media/filters/gpu_video_decoder.h (right): https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.h:139: std::list<DecodeCB> pending_decode_callbacks_; On 2014/04/17 01:06:47, xhwang wrote: > Usually decoders have decoding delay, e.g. for a decoding delay of 3: > > input 0 1 2 3 4 5 6 ... > output 0 1 2 3 ... > > Assuming kMaxInFlightDecodes > 3 (see my question to fischman@ below), it seems > we'll always have at least 3 pending decode_cb in pending_decode_callbacks_. The > result of this is that the effective max number of in-flight decodes are dropped > to kMaxInFlightDecodes - 3. This will be bad if decoding is slow and we really > want to have kMaxInFlightDecodes decodes in parallel. I think this can be fixed if VDA always returns something even if it cannot produce an output frame. With that, we can return kNotEnoughData (which is created for this very purpose), to DS so that DS can schedule another Decode call. Another possible solution is that in GVD, we always return kNotEnoughData for the first decoding_delay Decode() calls. But I guess we don't know decoding_delay in advance. Another possible solution is that maybe we can keep a permanent decode_cb_ in VideoDecoder (not a one-off cb passed in Decode() call). Whenever a decoder generates a decoded frame, it fires decode_cb_ to return the frame. This will actually work nicely for AudioDecoders where one input can result in multiple outputs. But this would be a big change and I need to think more about it...
https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... File media/filters/decoder_stream.cc (right): https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:89: state_ == STATE_ERROR || state_ == STATE_REINITIALIZING_DECODER) On 2014/04/17 01:06:47, xhwang wrote: > |state_| can also be STATE_PENDING_DEMUXER_READ. Imagine: > > DS::Read() { > Read satisfied because ready_output_buffers isn't empty() (line 106). > ReadFromDemuxerStream(); // line 123; state_ changed to > STATE_PENDING_DEMUXER_READ > } > > // Since previous Read() was satisfied. > > DS::Read() { > // Now state is STATE_PENDING_DEMUXER_READ. > } > > Actually you have the check against STATE_PENDING_DEMUXER_READ on line 122 :) Done. Also added a unittest that coverts this case. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:111: if (state_ == STATE_REINITIALIZING_DECODER) On 2014/04/17 01:06:47, xhwang wrote: > Might worth a comment how this could happen. Done. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:222: int buffers_in_queue = On 2014/04/17 01:06:47, xhwang wrote: > In media/ we use "buffer" for compressed data, e.g. DecoderBuffer. In this > class, we use "output" to refer to decoded output. Maybe we should rename > buffers_in_queue and ready_output_buffers... Renamed buffers_in_queue -> num_decodes ready_output_buffers_ -> ready_outputs_ https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:224: return buffers_in_queue < decoder_->GetMaxDecodeRequests(); On 2014/04/17 01:06:47, xhwang wrote: > Probably add a comment about why we choose to do this. My understanding is: if > decoding is slow and DS's buffers_in_queue is running low, then we can trigger > to do parallel decoding at the maximum speed. Done. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:330: } On 2014/04/17 01:06:47, xhwang wrote: > Now we could have multiple Decode() calls in flight, but we still only have one > |read_cb_|. That's why SatisfyRead() always assumes that the |read_cb_| is not > null. So when pending_decode_requests_ > 1 and we call DecoderStream::Reset(), I > assume OnDecodeOutputReady() will be called for multiple times before the > decoder is fully reset. In that case, we'll call SatisfyRead() multiple times, > which won't work. I think what we need to do is only SatisfyRead once. Done. Also added unittests for this case. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:351: AbortRead(); On 2014/04/17 01:06:47, xhwang wrote: > ditto. We only need to AbortRead() once. Removed AbortRead(). Reads are aborted from Reset() now. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:375: while ((extra_output = decoder_->GetDecodeOutput()) != NULL) { On 2014/04/17 01:06:47, xhwang wrote: > Now GetDecodeOutput() will always return NULL for GVD. So this function is only > used by the audio decoders (where one input can result in multiple outputs). We > may need to clean this up a bit in the future. Added TODO in video_decoder.h. https://codereview.chromium.org/239893002/diff/20001/media/filters/decoder_st... media/filters/decoder_stream.cc:456: if (state_ == STATE_NORMAL && CanDecodeAnotherBuffer()) { On 2014/04/17 01:06:47, xhwang wrote: > We set state_ to be STATE_NORMAL on line 410, so here state_ has to be > STATE_NORMAL? Done. https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.cc:327: return kMaxInFlightDecodes; On 2014/04/17 01:06:47, xhwang wrote: > [not related to this CL] > > fischman: Do you know what's the decoding delay of VDAs? Are they guaranteed to > be < kMaxInFlightDecodes? Up until this CL, we always schedule > kMaxInFlightDecodes decodes upon the first DS::Read() call. This seems to work. > But if we have a VDA that has decoding delay > kMaxInFlightDecodes, then we > won't get any decoded frames after kMaxInFlightDecodes decode calls, and DS will > not issue new decode calls, hence a deadlock. per e-mail thread VDA decoder shouldn't have any delay. https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.cc:476: CHECK(!pending_decode_callbacks_.empty()); On 2014/04/17 01:06:47, xhwang wrote: > This is catching a coding error, use DCHECK? Done. https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.cc:587: DeliverFrame(VideoFrame::CreateEOSFrame()); On 2014/04/17 01:06:47, xhwang wrote: > I suspect that after FlushDone, we'll still have unsatisfied decode_cb's. (See > comments above about decoding delay.) Added DCHECK_EQ(pending_decode_callbacks_.size(), 1U); https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... File media/filters/gpu_video_decoder.h (left): https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.h:186: std::list<scoped_refptr<VideoFrame> > ready_video_frames_; On 2014/04/17 01:06:47, xhwang wrote: > Drop this? Done. https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... File media/filters/gpu_video_decoder.h (right): https://codereview.chromium.org/239893002/diff/20001/media/filters/gpu_video_... media/filters/gpu_video_decoder.h:139: std::list<DecodeCB> pending_decode_callbacks_; I think Ami responded to this over e-mail. Decoder is supposed to return each frame as soon as it is encoded. On 2014/04/17 01:06:47, xhwang wrote: > Usually decoders have decoding delay, e.g. for a decoding delay of 3: > > input 0 1 2 3 4 5 6 ... > output 0 1 2 3 ... > > Assuming kMaxInFlightDecodes > 3 (see my question to fischman@ below), it seems > we'll always have at least 3 pending decode_cb in pending_decode_callbacks_. The > result of this is that the effective max number of in-flight decodes are dropped > to kMaxInFlightDecodes - 3. This will be bad if decoding is slow and we really > want to have kMaxInFlightDecodes decodes in parallel.
Sergey: please update reviewlog when you're ready for a real review or need one or more reviewers to take another look. On Tue, Apr 22, 2014 at 7:44 PM, <sergeyu@chromium.org> wrote: > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/decoder_stream.cc > File media/filters/decoder_stream.cc (right): > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/decoder_stream.cc#newcode89 > media/filters/decoder_stream.cc:89: state_ == STATE_ERROR || state_ == > STATE_REINITIALIZING_DECODER) > On 2014/04/17 01:06:47, xhwang wrote: > >> |state_| can also be STATE_PENDING_DEMUXER_READ. Imagine: >> > > DS::Read() { >> Read satisfied because ready_output_buffers isn't empty() (line 106). >> ReadFromDemuxerStream(); // line 123; state_ changed to >> STATE_PENDING_DEMUXER_READ >> } >> > > // Since previous Read() was satisfied. >> > > DS::Read() { >> // Now state is STATE_PENDING_DEMUXER_READ. >> } >> > > Actually you have the check against STATE_PENDING_DEMUXER_READ on line >> > 122 :) > > Done. Also added a unittest that coverts this case. > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/decoder_stream.cc#newcode111 > media/filters/decoder_stream.cc:111: if (state_ == > STATE_REINITIALIZING_DECODER) > On 2014/04/17 01:06:47, xhwang wrote: > >> Might worth a comment how this could happen. >> > > Done. > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/decoder_stream.cc#newcode222 > media/filters/decoder_stream.cc:222: int buffers_in_queue = > On 2014/04/17 01:06:47, xhwang wrote: > >> In media/ we use "buffer" for compressed data, e.g. DecoderBuffer. In >> > this > >> class, we use "output" to refer to decoded output. Maybe we should >> > rename > >> buffers_in_queue and ready_output_buffers... >> > > Renamed > buffers_in_queue -> num_decodes > ready_output_buffers_ -> ready_outputs_ > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/decoder_stream.cc#newcode224 > media/filters/decoder_stream.cc:224: return buffers_in_queue < > decoder_->GetMaxDecodeRequests(); > On 2014/04/17 01:06:47, xhwang wrote: > >> Probably add a comment about why we choose to do this. My >> > understanding is: if > > decoding is slow and DS's buffers_in_queue is running low, then we can >> > trigger > >> to do parallel decoding at the maximum speed. >> > > Done. > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/decoder_stream.cc#newcode330 > media/filters/decoder_stream.cc:330: } > > On 2014/04/17 01:06:47, xhwang wrote: > >> Now we could have multiple Decode() calls in flight, but we still only >> > have one > >> |read_cb_|. That's why SatisfyRead() always assumes that the >> > |read_cb_| is not > >> null. So when pending_decode_requests_ > 1 and we call >> > DecoderStream::Reset(), I > >> assume OnDecodeOutputReady() will be called for multiple times before >> > the > >> decoder is fully reset. In that case, we'll call SatisfyRead() >> > multiple times, > >> which won't work. I think what we need to do is only SatisfyRead once. >> > > Done. Also added unittests for this case. > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/decoder_stream.cc#newcode351 > media/filters/decoder_stream.cc:351: AbortRead(); > On 2014/04/17 01:06:47, xhwang wrote: > >> ditto. We only need to AbortRead() once. >> > > Removed AbortRead(). Reads are aborted from Reset() now. > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/decoder_stream.cc#newcode375 > media/filters/decoder_stream.cc:375: while ((extra_output = > decoder_->GetDecodeOutput()) != NULL) { > On 2014/04/17 01:06:47, xhwang wrote: > >> Now GetDecodeOutput() will always return NULL for GVD. So this >> > function is only > >> used by the audio decoders (where one input can result in multiple >> > outputs). We > >> may need to clean this up a bit in the future. >> > > Added TODO in video_decoder.h. > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/decoder_stream.cc#newcode456 > media/filters/decoder_stream.cc:456: if (state_ == STATE_NORMAL && > CanDecodeAnotherBuffer()) { > On 2014/04/17 01:06:47, xhwang wrote: > >> We set state_ to be STATE_NORMAL on line 410, so here state_ has to be >> STATE_NORMAL? >> > > Done. > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/gpu_video_decoder.cc > File media/filters/gpu_video_decoder.cc (right): > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/gpu_video_decoder.cc#newcode327 > media/filters/gpu_video_decoder.cc:327: return kMaxInFlightDecodes; > On 2014/04/17 01:06:47, xhwang wrote: > >> [not related to this CL] >> > > fischman: Do you know what's the decoding delay of VDAs? Are they >> > guaranteed to > >> be < kMaxInFlightDecodes? Up until this CL, we always schedule >> kMaxInFlightDecodes decodes upon the first DS::Read() call. This seems >> > to work. > >> But if we have a VDA that has decoding delay > kMaxInFlightDecodes, >> > then we > >> won't get any decoded frames after kMaxInFlightDecodes decode calls, >> > and DS will > >> not issue new decode calls, hence a deadlock. >> > > per e-mail thread VDA decoder shouldn't have any delay. > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/gpu_video_decoder.cc#newcode476 > media/filters/gpu_video_decoder.cc:476: > CHECK(!pending_decode_callbacks_.empty()); > On 2014/04/17 01:06:47, xhwang wrote: > >> This is catching a coding error, use DCHECK? >> > > Done. > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/gpu_video_decoder.cc#newcode587 > media/filters/gpu_video_decoder.cc:587: > DeliverFrame(VideoFrame::CreateEOSFrame()); > On 2014/04/17 01:06:47, xhwang wrote: > >> I suspect that after FlushDone, we'll still have unsatisfied >> > decode_cb's. (See > >> comments above about decoding delay.) >> > > Added DCHECK_EQ(pending_decode_callbacks_.size(), 1U); > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/gpu_video_decoder.h > File media/filters/gpu_video_decoder.h (left): > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/gpu_video_decoder.h#oldcode186 > media/filters/gpu_video_decoder.h:186: > std::list<scoped_refptr<VideoFrame> > ready_video_frames_; > On 2014/04/17 01:06:47, xhwang wrote: > >> Drop this? >> > > Done. > > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/gpu_video_decoder.h > File media/filters/gpu_video_decoder.h (right): > > https://codereview.chromium.org/239893002/diff/20001/ > media/filters/gpu_video_decoder.h#newcode139 > media/filters/gpu_video_decoder.h:139: std::list<DecodeCB> > pending_decode_callbacks_; > I think Ami responded to this over e-mail. Decoder is supposed to return > each frame as soon as it is encoded. > > > On 2014/04/17 01:06:47, xhwang wrote: > >> Usually decoders have decoding delay, e.g. for a decoding delay of 3: >> > > input 0 1 2 3 4 5 6 ... >> output 0 1 2 3 ... >> > > Assuming kMaxInFlightDecodes > 3 (see my question to fischman@ below), >> > it seems > >> we'll always have at least 3 pending decode_cb in >> > pending_decode_callbacks_. The > >> result of this is that the effective max number of in-flight decodes >> > are dropped > >> to kMaxInFlightDecodes - 3. This will be bad if decoding is slow and >> > we really > >> want to have kMaxInFlightDecodes decodes in parallel. >> > > https://codereview.chromium.org/239893002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/23 17:25:53, Ami Fischman wrote: > Sergey: please update reviewlog when you're ready for a real review or need > one or more reviewers to take another look. It is ready for review now.
LG and defer to xhwang for the real/final review. Thanks!
(FYI taking a look at this today -- was behind on my code reviews)
It looks good in general. Thanks! As discussed offline, parallel decoding cannot work with decoders that have decoding delay. We should have a comment/warning about this somewhere so that people don't misuse it. Also, you'll need to wait for scherkus's comments :) https://codereview.chromium.org/239893002/diff/100001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/239893002/diff/100001/media/base/video_decode... media/base/video_decoder.h:51: // GetMaxDecodeRequests() to get number of frames that may be decoded in s/frames/buffers https://codereview.chromium.org/239893002/diff/100001/media/base/video_decode... media/base/video_decoder.h:70: // TODO(xhwang): Remove this method. s/Remove/Revisit Since we still have 1:1 mapping b/w DecodeCB and input buffer, we'll still need this for audio. https://codereview.chromium.org/239893002/diff/100001/media/base/video_decode... media/base/video_decoder.h:95: // Returns how many frames that may be decoded in parallel. s/frame/buffer https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... File media/filters/decoder_stream.cc (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:233: // TODO(sergeyu): Is that the best way to throttle decoding requests? I don't see a better way to do that. The current approach makes sense to me. https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:309: if (pending_decode_requests_ == 0) This also depends on the fact that GVD doesn't have decoding delays, which may not be true in general. (We need a high level comment about decoding delay assumption somewhere.) https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:330: if (state_ == STATE_ERROR) DCHECK(!read_cb_.is_null())? https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:335: error_ = DECODE_ERROR; Shall we clear the ready_outputs_ in case of error? https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:343: error_ = DECRYPT_ERROR; ditto https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:350: if (!read_cb_.is_null()) kAborted must be a result of Reset(). In Reset() call we already cleared ready_outputs_. Also, any frame returned after Reset() won't be added to ready_outputs_ until Reset() is completed. So we should be able to DCHECK(ready_outputs_.empty()) here. WDYT? https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:423: if (state_ != STATE_PENDING_DEMUXER_READ) Can we DCHECK_EQ(state_, STATE_ERROR); DCHECK(read_cb_.is_null()); ? https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... File media/filters/decoder_stream.h (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.h:91: bool CanDecodeAnotherBuffer() const; nit: how about CanDecodeMore()? https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.h:169: Status error_; DECRYPT_ERROR is going to be deprecated. When state_ is ERROR, you can just return DECODE_ERROR in ReadCBs. https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.h:196: int pending_decode_requests_; This worth a separate comment. https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_demu... File media/filters/fake_demuxer_stream.h (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_demu... media/filters/fake_demuxer_stream.h:53: void SatisfyAndHoldRead(); SatisfyAndHoldNextRead()? https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... File media/filters/fake_video_decoder.cc (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:208: } else { DCHECK(held_decode_callbacks_.empty()); We should not satisfy a decode callback if there are decode cbs held in the queue. https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:216: if (!reset_cb_.IsNull()) { DCHECK(decoded_frames_.empty()); https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:220: if (static_cast<int>(decoded_frames_.size()) <= decoding_delay_ && The two cases we use |decoding_delay_| both involves static_cast. We may just define it as a size_t. https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:222: decode_cb.Run(kNotEnoughData, scoped_refptr<VideoFrame>()); Return early here then we don't need the "else" in the next line. https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... File media/filters/fake_video_decoder_unittest.cc (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... media/filters/fake_video_decoder_unittest.cc:24: : decoder_(new FakeVideoDecoder(kDecodingDelay, false, 1)), It'll be nice to also test with max_parallel_decoding_requests > 1. https://codereview.chromium.org/239893002/diff/100001/media/filters/gpu_video... File media/filters/gpu_video_decoder.h (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/gpu_video... media/filters/gpu_video_decoder.h:135: // Callbacks for the pending Decode requests (not more that s/that/than https://codereview.chromium.org/239893002/diff/100001/media/filters/video_fra... File media/filters/video_frame_stream_unittest.cc (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:371: VideoFrameStreamTestParams(true, false, 0, 3))); Add a comment that we don't support parallel decode with decoding delays. https://codereview.chromium.org/239893002/diff/100001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:403: Reset(); Is this Reset() necessary? https://codereview.chromium.org/239893002/diff/100001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:412: ReadUntilPending(); It'd be nice to verify that all decoded frames are read even though the demuxer stream is blocked.
https://codereview.chromium.org/239893002/diff/100001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/239893002/diff/100001/media/base/video_decode... media/base/video_decoder.h:51: // GetMaxDecodeRequests() to get number of frames that may be decoded in On 2014/04/25 00:36:03, xhwang wrote: > s/frames/buffers Done. https://codereview.chromium.org/239893002/diff/100001/media/base/video_decode... media/base/video_decoder.h:70: // TODO(xhwang): Remove this method. On 2014/04/25 00:36:03, xhwang wrote: > s/Remove/Revisit > > Since we still have 1:1 mapping b/w DecodeCB and input buffer, we'll still need > this for audio. Done. I think the right way to fix it is to pass multiple output buffers in DecodeCB - so this function won't be needed. https://codereview.chromium.org/239893002/diff/100001/media/base/video_decode... media/base/video_decoder.h:95: // Returns how many frames that may be decoded in parallel. On 2014/04/25 00:36:03, xhwang wrote: > s/frame/buffer Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... File media/filters/decoder_stream.cc (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:233: // TODO(sergeyu): Is that the best way to throttle decoding requests? On 2014/04/25 00:36:03, xhwang wrote: > I don't see a better way to do that. The current approach makes sense to me. removed TODO https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:309: if (pending_decode_requests_ == 0) On 2014/04/25 00:36:03, xhwang wrote: > This also depends on the fact that GVD doesn't have decoding delays, which may > not be true in general. (We need a high level comment about decoding delay > assumption somewhere.) Added condition in VideoDecoder comment requiring that |decode_cb| is called in the same order in which the decoder gets the requests, i.e. frames cannot be decoded out of order. That's what I think is important here. https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:330: if (state_ == STATE_ERROR) On 2014/04/25 00:36:03, xhwang wrote: > DCHECK(!read_cb_.is_null())? I think you meant "DCHECK(read_cb_.is_null())" Done https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:335: error_ = DECODE_ERROR; On 2014/04/25 00:36:03, xhwang wrote: > Shall we clear the ready_outputs_ in case of error? Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:343: error_ = DECRYPT_ERROR; On 2014/04/25 00:36:03, xhwang wrote: > ditto Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:350: if (!read_cb_.is_null()) On 2014/04/25 00:36:03, xhwang wrote: > kAborted must be a result of Reset(). In Reset() call we already cleared > ready_outputs_. Also, any frame returned after Reset() won't be added to > ready_outputs_ until Reset() is completed. So we should be able to > DCHECK(ready_outputs_.empty()) here. WDYT? There are some tests that return kAborted from decoder without Reset() being called. See [Audio|Video]RendererImplTest.AbortPendingRead_* . So I assume there is a case when stream can be aborted by the decoder. Do you think those tests are incorrect and should be fixed? https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:423: if (state_ != STATE_PENDING_DEMUXER_READ) On 2014/04/25 00:36:03, xhwang wrote: > Can we > DCHECK_EQ(state_, STATE_ERROR); > DCHECK(read_cb_.is_null()); > ? Done. state_ can be either STATE_ERROR or STATE_STOPPED. https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... File media/filters/decoder_stream.h (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.h:91: bool CanDecodeAnotherBuffer() const; On 2014/04/25 00:36:03, xhwang wrote: > nit: how about CanDecodeMore()? Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.h:169: Status error_; On 2014/04/25 00:36:03, xhwang wrote: > DECRYPT_ERROR is going to be deprecated. When state_ is ERROR, you can just > return DECODE_ERROR in ReadCBs. Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.h:196: int pending_decode_requests_; On 2014/04/25 00:36:03, xhwang wrote: > This worth a separate comment. Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_demu... File media/filters/fake_demuxer_stream.h (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_demu... media/filters/fake_demuxer_stream.h:53: void SatisfyAndHoldRead(); On 2014/04/25 00:36:03, xhwang wrote: > SatisfyAndHoldNextRead()? Renamed to SatisfyReadAndHoldNext() https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... File media/filters/fake_video_decoder.cc (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:208: } else { On 2014/04/25 00:36:03, xhwang wrote: > DCHECK(held_decode_callbacks_.empty()); > > We should not satisfy a decode callback if there are decode cbs held in the > queue. Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:216: if (!reset_cb_.IsNull()) { On 2014/04/25 00:36:03, xhwang wrote: > DCHECK(decoded_frames_.empty()); Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:220: if (static_cast<int>(decoded_frames_.size()) <= decoding_delay_ && On 2014/04/25 00:36:03, xhwang wrote: > The two cases we use |decoding_delay_| both involves static_cast. We may just > define it as a size_t. Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:222: decode_cb.Run(kNotEnoughData, scoped_refptr<VideoFrame>()); On 2014/04/25 00:36:03, xhwang wrote: > Return early here then we don't need the "else" in the next line. Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... File media/filters/fake_video_decoder_unittest.cc (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/fake_vide... media/filters/fake_video_decoder_unittest.cc:24: : decoder_(new FakeVideoDecoder(kDecodingDelay, false, 1)), On 2014/04/25 00:36:03, xhwang wrote: > It'll be nice to also test with max_parallel_decoding_requests > 1. Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/gpu_video... File media/filters/gpu_video_decoder.h (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/gpu_video... media/filters/gpu_video_decoder.h:135: // Callbacks for the pending Decode requests (not more that On 2014/04/25 00:36:03, xhwang wrote: > s/that/than Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/video_fra... File media/filters/video_frame_stream_unittest.cc (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:371: VideoFrameStreamTestParams(true, false, 0, 3))); On 2014/04/25 00:36:03, xhwang wrote: > Add a comment that we don't support parallel decode with decoding delays. There is nothing that disallows a delay in parallel decoders. Added a test with kDecodingDelay and parallel_decoding = 3 https://codereview.chromium.org/239893002/diff/100001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:403: Reset(); On 2014/04/25 00:36:03, xhwang wrote: > Is this Reset() necessary? Done. https://codereview.chromium.org/239893002/diff/100001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:412: ReadUntilPending(); On 2014/04/25 00:36:03, xhwang wrote: > It'd be nice to verify that all decoded frames are read even though the demuxer > stream is blocked. Done.
scherkus, xhwang: ping
On 2014/04/29 01:14:33, Sergey Ulanov wrote: > scherkus, xhwang: ping Sorry for the delay. I am looking right now.
Looking pretty good! Just a few comments on tests and comments. scherkus@: You should skim this CL :) https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... File media/filters/decoder_stream.cc (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:309: if (pending_decode_requests_ == 0) On 2014/04/26 00:59:29, Sergey Ulanov wrote: > On 2014/04/25 00:36:03, xhwang wrote: > > This also depends on the fact that GVD doesn't have decoding delays, which may > > not be true in general. (We need a high level comment about decoding delay > > assumption somewhere.) > > Added condition in VideoDecoder comment requiring that |decode_cb| is called in > the same order in which the decoder gets the requests, i.e. frames cannot be > decoded out of order. That's what I think is important here. By "decoding delay", I mean the case where VideoDecoder will not return a decoded frame until more buffers has been fed to the decoder. For example: input: I P B B P B B ... output: I B B P B B P ... Here's what I was worrying about: Demuxed buffer: 1 2 3 ConfigChange Then DS will make the following calls: Decode(1): no frame returned Decode(2): frame 1 returned Decode(3): frame 2 returned FlushDecoder() Where FlushDecoder() will do nothing because pending_decode_requests_ is not zero (frame 3 not returned). But without flushing the decoder, the decoder will not return frame 3, hence a deadlock. Currently we don't have a decoder that does this so we are fine: 1, GVD doesn't have decoding delay. 2, Decoders that have decoding delay always return kNotEnoughData in DecodeCB so that we don't accumulate DecodeCB due to decoding delay. Maybe we should add a comment in VideoDecoder that the decoder must call DecodeCB some time after Decode() call even if there are no further inputs. In other words, decoders must call DecodeCB with a frame if it can produce one, or with a kNotEnoughData if it cannot (e.g. due to decoding delay). WDYT? https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:350: if (!read_cb_.is_null()) On 2014/04/26 00:59:29, Sergey Ulanov wrote: > On 2014/04/25 00:36:03, xhwang wrote: > > kAborted must be a result of Reset(). In Reset() call we already cleared > > ready_outputs_. Also, any frame returned after Reset() won't be added to > > ready_outputs_ until Reset() is completed. So we should be able to > > DCHECK(ready_outputs_.empty()) here. WDYT? > > There are some tests that return kAborted from decoder without Reset() being > called. See [Audio|Video]RendererImplTest.AbortPendingRead_* . So I assume there > is a case when stream can be aborted by the decoder. Do you think those tests > are incorrect and should be fixed? You are right. kAborted can also be a result of DemuxerStream::kAborted. https://codereview.chromium.org/239893002/diff/120001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/239893002/diff/120001/media/base/video_decode... media/base/video_decoder.h:51: // GetMaxDecodeRequests() to get number of parallel that may be decoded in s/parallel/buffers ? https://codereview.chromium.org/239893002/diff/120001/media/filters/fake_vide... File media/filters/fake_video_decoder_unittest.cc (right): https://codereview.chromium.org/239893002/diff/120001/media/filters/fake_vide... media/filters/fake_video_decoder_unittest.cc:283: } Just to double check? EXPECT_EQ(pending_decode_requests_, max_decode_requests) https://codereview.chromium.org/239893002/diff/120001/media/filters/fake_vide... media/filters/fake_video_decoder_unittest.cc:285: } I like this test. Thank you! https://codereview.chromium.org/239893002/diff/120001/media/filters/fake_vide... media/filters/fake_video_decoder_unittest.cc:287: remove extra empty line https://codereview.chromium.org/239893002/diff/120001/media/filters/video_fra... File media/filters/video_frame_stream_unittest.cc (right): https://codereview.chromium.org/239893002/diff/120001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:408: int demuxed_frames = 0; nit: s/frame/buffer Actually you can just use FakeDemuxerStream::num_buffers_returned(). https://codereview.chromium.org/239893002/diff/120001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:420: EXPECT_EQ(std::max(GetParam().decoding_delay - 1, 1), demuxed_frames); why? For example: input: 1 2 3 ... output: 1 2 ... decoding_delay is 1 so std::max(GetParam().decoding_delay - 1, 1) is 1. but now we have one frame decoded, so we should have 2 demuxed buffers. You can change the test parameters to repro this. https://codereview.chromium.org/239893002/diff/120001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:425: ReadUntilPending(); My original proposal is to add a test where we have: 1, multiple decode calls 2, multiple frames decoded and stored in ready_outputs_ 3, then we have a demuxer read blocking 4, VideoFrameStream::Read() We want to make sure the the last VFS::Read() can still be satisfied without satisfying the blocking demuxer read. I want to test it because it's one of the motivations of this CL. The current test doesn't cover this case. I verified that we never accumulate internal buffers with the current settings.
BTW, feel free to play with more combinations of decoding_delay, parallel_deocodes etc to make sure the code is robust.
https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... File media/filters/decoder_stream.cc (right): https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s... media/filters/decoder_stream.cc:309: if (pending_decode_requests_ == 0) Ah, I see your point. Yes, decoders must never wait for following Decode() call before calling decode_cb. I've added comment about it in video_decoder.h. On 2014/04/29 20:01:26, xhwang wrote: > On 2014/04/26 00:59:29, Sergey Ulanov wrote: > > On 2014/04/25 00:36:03, xhwang wrote: > > > This also depends on the fact that GVD doesn't have decoding delays, which > may > > > not be true in general. (We need a high level comment about decoding delay > > > assumption somewhere.) > > > > Added condition in VideoDecoder comment requiring that |decode_cb| is called > in > > the same order in which the decoder gets the requests, i.e. frames cannot be > > decoded out of order. That's what I think is important here. > > By "decoding delay", I mean the case where VideoDecoder will not return a > decoded frame until more buffers has been fed to the decoder. > > For example: > input: I P B B P B B ... > output: I B B P B B P ... > > Here's what I was worrying about: > > Demuxed buffer: 1 2 3 ConfigChange > > Then DS will make the following calls: > Decode(1): no frame returned > Decode(2): frame 1 returned > Decode(3): frame 2 returned > FlushDecoder() > > Where FlushDecoder() will do nothing because pending_decode_requests_ is not > zero (frame 3 not returned). But without flushing the decoder, the decoder will > not return frame 3, hence a deadlock. > > Currently we don't have a decoder that does this so we are fine: > 1, GVD doesn't have decoding delay. > 2, Decoders that have decoding delay always return kNotEnoughData in DecodeCB so > that we don't accumulate DecodeCB due to decoding delay. > > Maybe we should add a comment in VideoDecoder that the decoder must call > DecodeCB some time after Decode() call even if there are no further inputs. In > other words, decoders must call DecodeCB with a frame if it can produce one, or > with a kNotEnoughData if it cannot (e.g. due to decoding delay). > > WDYT? https://codereview.chromium.org/239893002/diff/120001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/239893002/diff/120001/media/base/video_decode... media/base/video_decoder.h:51: // GetMaxDecodeRequests() to get number of parallel that may be decoded in On 2014/04/29 20:01:26, xhwang wrote: > s/parallel/buffers ? Done. https://codereview.chromium.org/239893002/diff/120001/media/filters/fake_vide... File media/filters/fake_video_decoder_unittest.cc (right): https://codereview.chromium.org/239893002/diff/120001/media/filters/fake_vide... media/filters/fake_video_decoder_unittest.cc:283: } On 2014/04/29 20:01:26, xhwang wrote: > Just to double check? > > EXPECT_EQ(pending_decode_requests_, max_decode_requests) Done. https://codereview.chromium.org/239893002/diff/120001/media/filters/fake_vide... media/filters/fake_video_decoder_unittest.cc:287: On 2014/04/29 20:01:26, xhwang wrote: > remove extra empty line Done. https://codereview.chromium.org/239893002/diff/120001/media/filters/video_fra... File media/filters/video_frame_stream_unittest.cc (right): https://codereview.chromium.org/239893002/diff/120001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:408: int demuxed_frames = 0; On 2014/04/29 20:01:26, xhwang wrote: > nit: s/frame/buffer > > Actually you can just use FakeDemuxerStream::num_buffers_returned(). Done. https://codereview.chromium.org/239893002/diff/120001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:420: EXPECT_EQ(std::max(GetParam().decoding_delay - 1, 1), demuxed_frames); On 2014/04/29 20:01:26, xhwang wrote: > why? For example: > input: 1 2 3 ... > output: 1 2 ... > > decoding_delay is 1 > so std::max(GetParam().decoding_delay - 1, 1) is 1. > but now we have one frame decoded, so we should have 2 demuxed buffers. > You can change the test parameters to repro this. Yes, you are right. This should be std::min(GetParam().decoding_delay + 1, kNumBuffersInOneConfig + 1); https://codereview.chromium.org/239893002/diff/120001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:425: ReadUntilPending(); On 2014/04/29 20:01:26, xhwang wrote: > My original proposal is to add a test where we have: > 1, multiple decode calls > 2, multiple frames decoded and stored in ready_outputs_ > 3, then we have a demuxer read blocking > 4, VideoFrameStream::Read() > We want to make sure the the last VFS::Read() can still be satisfied without > satisfying the blocking demuxer read. > > I want to test it because it's one of the motivations of this CL. > > The current test doesn't cover this case. I verified that we never accumulate > internal buffers with the current settings. That's a good point. Added another test now.
Thanks! Only one comment about the new test. https://codereview.chromium.org/239893002/diff/140001/media/filters/video_fra... File media/filters/video_frame_stream_unittest.cc (right): https://codereview.chromium.org/239893002/diff/140001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:462: // still blocked. Not sure if I understand this test correctly. The pending VFS::Read should already be satisfied here since we are out of the while(pending_read_) loop. Is that correct? Should we SatisfySingleDecode() here and ReadOneFrame(). Then check that the read is satisfied without the need to satisfy the demuxer read?
https://codereview.chromium.org/239893002/diff/140001/media/filters/video_fra... File media/filters/video_frame_stream_unittest.cc (right): https://codereview.chromium.org/239893002/diff/140001/media/filters/video_fra... media/filters/video_frame_stream_unittest.cc:462: // still blocked. On 2014/04/30 21:36:52, xhwang wrote: > Not sure if I understand this test correctly. > > The pending VFS::Read should already be satisfied here since we are out of the > while(pending_read_) loop. Is that correct? > > Should we SatisfySingleDecode() here and ReadOneFrame(). Then check that the > read is satisfied without the need to satisfy the demuxer read? Yes, you are right. I forgot to add ReadUntilPending() above. Also fixed FakeVideoDecoder behavior with decode_delay > 0 when there are multiple decode requests.
Thanks for the hard work! LGTM % nits https://codereview.chromium.org/239893002/diff/160001/media/filters/fake_vide... File media/filters/fake_video_decoder.cc (right): https://codereview.chromium.org/239893002/diff/160001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:58: decoding_delay_ + max_parallel_decoding_requests_); s/max_parallel_decoding_requests_/held_decode_callbacks_.size() ? https://codereview.chromium.org/239893002/diff/160001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:157: void FakeVideoDecoder::SatisfySingleDecode() { We need to add tests to cover SatisfySingleDecode(). This is one I used when reviewing this CL: TEST_P(FakeVideoDecoderTest, ReadWithHold_DecodingDelay) { Initialize(); // Hold all decodes and satisfy one decode at a time. decoder_->HoldDecode(); int num_decodes_satisfied = 0; while (num_decoded_frames_ == 0) { while (pending_decode_requests_ < decoder_->GetMaxDecodeRequests()) Decode(); decoder_->SatisfySingleDecode(); ++num_decodes_satisfied; message_loop_.RunUntilIdle(); } DCHECK_EQ(num_decoded_frames_, 1); DCHECK_EQ(num_decodes_satisfied, kDecodingDelay + 1); } https://codereview.chromium.org/239893002/diff/160001/media/filters/fake_vide... File media/filters/fake_video_decoder_unittest.cc (right): https://codereview.chromium.org/239893002/diff/160001/media/filters/fake_vide... media/filters/fake_video_decoder_unittest.cc:226: ::testing::Values(1, 3)); To make the test name more readable, how about: INSTANTIATE_TEST_CASE_P(NoParallelDecode, FakeVideoDecoderTest, ::testing::Values(1)); INSTANTIATE_TEST_CASE_P(ParallelDecode, FakeVideoDecoderTest, ::testing::Values(3));
Andrew, should I wait for your review or can I land it now? https://codereview.chromium.org/239893002/diff/160001/media/filters/fake_vide... File media/filters/fake_video_decoder.cc (right): https://codereview.chromium.org/239893002/diff/160001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:58: decoding_delay_ + max_parallel_decoding_requests_); On 2014/05/01 17:40:35, xhwang wrote: > s/max_parallel_decoding_requests_/held_decode_callbacks_.size() ? Done. https://codereview.chromium.org/239893002/diff/160001/media/filters/fake_vide... media/filters/fake_video_decoder.cc:157: void FakeVideoDecoder::SatisfySingleDecode() { On 2014/05/01 17:40:35, xhwang wrote: > We need to add tests to cover SatisfySingleDecode(). > > This is one I used when reviewing this CL: > > TEST_P(FakeVideoDecoderTest, ReadWithHold_DecodingDelay) { > Initialize(); > > // Hold all decodes and satisfy one decode at a time. > decoder_->HoldDecode(); > int num_decodes_satisfied = 0; > while (num_decoded_frames_ == 0) { > while (pending_decode_requests_ < decoder_->GetMaxDecodeRequests()) > Decode(); > decoder_->SatisfySingleDecode(); > ++num_decodes_satisfied; > message_loop_.RunUntilIdle(); > } > > DCHECK_EQ(num_decoded_frames_, 1); > DCHECK_EQ(num_decodes_satisfied, kDecodingDelay + 1); > } Done. Thanks for writing the test for me :) https://codereview.chromium.org/239893002/diff/160001/media/filters/fake_vide... File media/filters/fake_video_decoder_unittest.cc (right): https://codereview.chromium.org/239893002/diff/160001/media/filters/fake_vide... media/filters/fake_video_decoder_unittest.cc:226: ::testing::Values(1, 3)); On 2014/05/01 17:40:35, xhwang wrote: > To make the test name more readable, how about: > > INSTANTIATE_TEST_CASE_P(NoParallelDecode, > FakeVideoDecoderTest, > ::testing::Values(1)); > INSTANTIATE_TEST_CASE_P(ParallelDecode, > FakeVideoDecoderTest, > ::testing::Values(3)); Done.
After testing this change more I discovered that it break H.264 decoding on daisy_spring devices. So I reverted GpuVideoDecoder changes in this CL. I'll upload a separate CL for GpuVideoDecoder changes.
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/239893002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/239893002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/239893002/240001
Message was sent while issue was closed.
Change committed as 268140 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
