Simplify VideoDecodeEngine interface by making everything synchronous.

Simplify VideoDecodeEngine interface by making everything synchronous.

Although I plan to remove VideoDecodeEngine entirely it requires detangling some of the code first.

Other noteworthy changes:
  - It's no longer valid to call VideoFrameReady(NULL), instead FFmpegVideoDecoder will raise an error the moment it finds one
  - Buffer recycling has been vanquished (for now), with video frames always allocated in the decoder
  - Produce/ConsumeVideoFrame() has been replaced by Read()
  - Video decode byte statistics are only updated if more than 0 bytes were decoded
  - FFmpegVideoDecodeEngine no longer attempts to preroll

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108612

[scherkus (not reviewing), 2011-10-28 16:54:03]

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:168: scoped_refptr<VideoFrame>
video_frame;
I'm not entirely sure if I should attempt to keep frames from being re-allocated

It would mean continuing to do a little bit of buffer recycling inside of
VideoRendererBase so the support is there today I just need to hold onto
VideoFrame pointers inside FFmpegVideoDecoder

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:227: // frame to be re-allocated on the
fly.
this is where we could either:
1) cache the video frames locally or if we wanted to get fancy
2) use currying to carry the video_frame object via a Read() to DemuxerStream

I'm hesitant of (2) since I'm not 100% sure how object lifetimes would work out

http://codereview.chromium.org/8417019/diff/6001/media/video/ffmpeg_video_dec...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/8417019/diff/6001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:190: scoped_refptr<VideoFrame>*
video_frame) {
this pointer-to-a-pointer business is a bit silly but it works

I wonder if we could extend VideoFrame to support re-allocation itself --
probably not a change for this CL

[acolwell GONE FROM CHROMIUM, 2011-10-28 17:38:47]

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:58: callback.Run();
I'd like to see callback converted to a PipelineStatusCB. Do it now or add a
TODO so this can happen in another CL.

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:168: scoped_refptr<VideoFrame>
video_frame;
I think we should remove the buffer recycling for now and add a new
implementation back in if we need to.

On 2011/10/28 16:54:04, scherkus wrote:
> I'm not entirely sure if I should attempt to keep frames from being
re-allocated
> 
> It would mean continuing to do a little bit of buffer recycling inside of
> VideoRendererBase so the support is there today I just need to hold onto
> VideoFrame pointers inside FFmpegVideoDecoder

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:227: // frame to be re-allocated on the
fly.
Remove the video_frame pointer for now. I want to remove all of the frame
recycling code. Like I said before I think this can be best handled with a
factory and this code would never need to know that buffer recycling is going
on.

On 2011/10/28 16:54:04, scherkus wrote:
> this is where we could either:
> 1) cache the video frames locally or if we wanted to get fancy
> 2) use currying to carry the video_frame object via a Read() to DemuxerStream
> 
> I'm hesitant of (2) since I'm not 100% sure how object lifetimes would work
out

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.h:22: : public VideoDecoder {
fits on line above?

http://codereview.chromium.org/8417019/diff/6001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/8417019/diff/6001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:370: statistics_callback_.Run(statistics);
Do we want to include end of stream or empty frames in this count?

http://codereview.chromium.org/8417019/diff/6001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:381: if (!frame->IsEndOfStream())
Why don't end of stream frames decrement the pending_read_ count? Aren't they
are sult of a read?

http://codereview.chromium.org/8417019/diff/6001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:390: DCHECK_LE(frames_queue_done_.size(),
Remove this queue. Frame recycling must die.

http://codereview.chromium.org/8417019/diff/6001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:451: void
VideoRendererBase::ReadInput(scoped_refptr<VideoFrame> frame) {
Remove frame parameter now that we aren't doing buffer recycling.

http://codereview.chromium.org/8417019/diff/6001/media/video/ffmpeg_video_dec...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/8417019/diff/6001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:190: scoped_refptr<VideoFrame>*
video_frame) {
Why not just use a scoped_refptr& ?
On 2011/10/28 16:54:04, scherkus wrote:
> this pointer-to-a-pointer business is a bit silly but it works
> 
> I wonder if we could extend VideoFrame to support re-allocation itself --
> probably not a change for this CL

[scherkus (not reviewing), 2011-11-01 04:18:26]

I realize this patch will be complicated to review but it's conceptually much
simpler:
  1) Frames are always allocated in the decoder
  2) Only one decode read at a time
  3) VRB always tries to request more frames to fill up its queue


We have a rough plan for (1) but I'd be interested in hearing your thoughts on
(2).

In my opinion I'm cool w/ (2) as it'll make for an easier transition to getting
rid of VDEngine and moving the decoders into renderers.

As for (3) it's a little quirky (AttemptFlush/AttemptRead) and could be cleaner
but I'd rather tackle that class separately.

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:58: callback.Run();
On 2011/10/28 17:38:47, acolwell wrote:
> I'd like to see callback converted to a PipelineStatusCB. Do it now or add a
> TODO so this can happen in another CL.

TODO added in filters.h

there are a few related filters.h TODOs I want to do (scoped_refptr<> for
buffers) so I'll tackle them in a future CL

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:168: scoped_refptr<VideoFrame>
video_frame;
On 2011/10/28 17:38:47, acolwell wrote:
> I think we should remove the buffer recycling for now and add a new
> implementation back in if we need to.
> 
> On 2011/10/28 16:54:04, scherkus wrote:
> > I'm not entirely sure if I should attempt to keep frames from being
> re-allocated
> > 
> > It would mean continuing to do a little bit of buffer recycling inside of
> > VideoRendererBase so the support is there today I just need to hold onto
> > VideoFrame pointers inside FFmpegVideoDecoder
> 

as discussed offline it looks like there was no way out of it!

I changed it to one-read-request-at-a-time -- let me know if you have thoughts
otherwise

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:227: // frame to be re-allocated on the
fly.
On 2011/10/28 17:38:47, acolwell wrote:
> Remove the video_frame pointer for now. I want to remove all of the frame
> recycling code. Like I said before I think this can be best handled with a
> factory and this code would never need to know that buffer recycling is going
> on.
> 
> On 2011/10/28 16:54:04, scherkus wrote:
> > this is where we could either:
> > 1) cache the video frames locally or if we wanted to get fancy
> > 2) use currying to carry the video_frame object via a Read() to
DemuxerStream
> > 
> > I'm hesitant of (2) since I'm not 100% sure how object lifetimes would work
> out
> 

Done.

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/8417019/diff/6001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.h:22: : public VideoDecoder {
On 2011/10/28 17:38:47, acolwell wrote:
> fits on line above?

Done.

http://codereview.chromium.org/8417019/diff/6001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/8417019/diff/6001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:370: statistics_callback_.Run(statistics);
On 2011/10/28 17:38:47, acolwell wrote:
> Do we want to include end of stream or empty frames in this count?

Moved below -- also shouldn't the decoder increment this?

http://codereview.chromium.org/8417019/diff/6001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:381: if (!frame->IsEndOfStream())
On 2011/10/28 17:38:47, acolwell wrote:
> Why don't end of stream frames decrement the pending_read_ count? Aren't they
> are sult of a read?

Done.

http://codereview.chromium.org/8417019/diff/6001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:390: DCHECK_LE(frames_queue_done_.size(),
On 2011/10/28 17:38:47, acolwell wrote:
> Remove this queue. Frame recycling must die.

Done.

http://codereview.chromium.org/8417019/diff/6001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:451: void
VideoRendererBase::ReadInput(scoped_refptr<VideoFrame> frame) {
On 2011/10/28 17:38:47, acolwell wrote:
> Remove frame parameter now that we aren't doing buffer recycling.

Done.

http://codereview.chromium.org/8417019/diff/6001/media/video/ffmpeg_video_dec...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/8417019/diff/6001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:190: scoped_refptr<VideoFrame>*
video_frame) {
On 2011/10/28 17:38:47, acolwell wrote:
> Why not just use a scoped_refptr& ?
> On 2011/10/28 16:54:04, scherkus wrote:
> > this pointer-to-a-pointer business is a bit silly but it works
> > 
> > I wonder if we could extend VideoFrame to support re-allocation itself --
> > probably not a change for this CL
> 

against style guide :\

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:217: // what?
FYI

[acolwell GONE FROM CHROMIUM, 2011-11-01 18:31:03]

Initial pass of comments.

http://codereview.chromium.org/8417019/diff/8001/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/8001/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder.cc:58: frame_ready_cb_.Reset();  // is
this needed?
I don't think this is needed. As far as I know Initialize() is only called once
at the beginning.

http://codereview.chromium.org/8417019/diff/8001/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder.cc:174:
base::TimeDelta::FromMilliseconds(30));
Do we really want to hard code 30ms for the duration? Are we guaranteed that we
will always produce frames at 33.33fps or less?

http://codereview.chromium.org/8417019/diff/8001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/base/filters.h#newcode176
media/base/filters.h:176: typedef
base::Callback<void(scoped_refptr<VideoFrame>)> FrameReadyCB;
Rename to ReadCB or ReadCallback? This is just to avoid confusion when looking
at other code. read_cb_ is a little clearer than frame_ready_cb_ when trying to
evaluate where the callback came from.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:217: // what?
On 2011/11/01 04:18:26, scherkus wrote:
> FYI
Will the caller ever block progress if it doesn't get called back here? If so
then it is probably best to just send empty frames back. 

At some point we might want to change the callback signature to include a
PipelineStatus so we could report errors for calls made in an invalid state or
at EOS.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:244: // TODO(scherkus): change
DemuxerStream::Read() to use scoped_refptr<> for
This TODO should be in demuxer_stream.h

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder_unittest.cc:320: // least one successful
decode but we don't expect ConsumeVideoFrame() to be
Is this comment still accurate?

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder_unittest.cc:326:
decoder_->Read(frame_ready_cb_);
We just drop these calls? Is there any code in the renderer that waits on reads
to complete before flushing/pausing/stopping/seeking?

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder_unittest.cc:356:
DecodeIFrameThenTestFile("vp8-I-frame-160x240", 160, 240);
Frame size changes are actually working now?

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder_unittest.cc:363: // 
DecodeIFrameThenTestFile("vp8-I-frame-320x480", 320, 480);
If frame size changes are working now you may want to try uncommenting this test
and the one above to see if they work now.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (left):

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:383:
EnterErrorState_Locked(PIPELINE_ERROR_DECODE);
Why not keep this?

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:79: if (thread_) {
Given the code below shouldn't this be (thread_ != base::kNullThreadHandle) ?

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:87: if (old_thread_handle)
ditto

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:386: if (state_ == kPaused) {
Can't this happen based on the "Prerolling while paused" comment below? This
looks like it will regress that feature.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:470: if (!pending_paint_ && !pending_read_)
{
I think this may be where you can get into trouble if the decoder drops
callbacks after a decode error. You'll never exit kFlushing if that happens.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:506: void
VideoRendererBase::EnterErrorState_Locked(PipelineStatus status) {
assuming you don't reinstate the call in FrameReady(), I think you can remove
this method since the only other call sites are in Initialize(). 

There is enough code here that makes me think the FrameReady() call is pretty
important.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:557: DCHECK(!pending_read_);
Looking at the code this doesn't seem like a safe assumption especially if a
decoder error occurs.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base_unittest.cc:277: VideoDecoder::FrameReadyCB
pending_read_;
Rename to read_cb_?

[Ami GONE FROM CHROMIUM, 2011-11-01 22:17:40]

I love this CL!

http://codereview.chromium.org/8417019/diff/8001/media/base/filters.h
File media/base/filters.h (left):

http://codereview.chromium.org/8417019/diff/8001/media/base/filters.h#oldcode196
media/base/filters.h:196: // TODO(scherkus): name this ConsumeVideoFrame() once
we fix the TODO in
I, for one, welcome our new reduced-craxy overlords.

http://codereview.chromium.org/8417019/diff/8001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/base/filters.h#newcode176
media/base/filters.h:176: typedef
base::Callback<void(scoped_refptr<VideoFrame>)> FrameReadyCB;
On 2011/11/01 18:31:03, acolwell wrote:
> Rename to ReadCB or ReadCallback? This is just to avoid confusion when looking
> at other code. read_cb_ is a little clearer than frame_ready_cb_ when trying
to
> evaluate where the callback came from.

FWIW, we decided a long time ago to use *CB for the new-style callbacks.  I hope
we keep that.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:124: void
FFmpegVideoDecoder::DoDecodeBuffer(const scoped_refptr<Buffer>& buffer) {
Why have this be a separate function?  It makes DecodeBuffer() an empty shell.

(given the ugliness of having to trampoline explicitly, I prefer doing it
in-line rather than having extra methods)

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:153: //     Any time Flush() is called.
I'm surprised a Flush() can recover from a "catastrophic failure".  Perhaps
downgrade that phrase to "decode error"?

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:210: CHECK(!callback.is_null());
Here and below s/CHECK/DCHECK/?

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:217: // what?
I say drop the callback.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.h:36: virtual void Read(const FrameReadyCB&
callback); OVERRIDE;
This is part of some other CL, right?  Note that fact in the CL description?

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:374: CHECK(pending_read_);
DCHECK?

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:378: // Decoder could reach seek state
before our Seek() get called.
s/get/got/

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:381: CHECK(false) << "I don't think this is
possible";
CHECK(false) is always better spelled LOG(FATAL)
But if you really don't think so why not replace this clause & comment with
DCHECK_NE(state_, kFlushed);
?

Ditto below.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:439: // when Seek() is eventually called.
English.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.h:177: // before executing the flush callback;
We decrement it each time we receive
decrement/increment terminology is misleading in bool context.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base_unittest.cc (left):

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base_unittest.cc:347: TEST_F(VideoRendererBaseTest,
Error_Playing) {
Why are these tests no longer needed?

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:132: //   2) End of stream was reached
and all internal frames have been output
I worry that these are things a client needs to know and thus belong in the
public .h API, not here.

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:144: LOG(ERROR) << "Video frame was
produced yet has invalid frame data.";
*video_frame=NULL;
?

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:149:
ReallocateVideoFrameIfNecessary(video_frame);
If you take my suggestion about INVALID below, make sure to check *video_frame
for NULLness and error out.

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:191: VideoFrame::Format format =
PixelFormatToVideoFormat(codec_context_->pix_fmt);
If you checked for VideoFrame::INVALID here and returned *video_frame=NULL here
you'd be fixing http://crbug.com/101803
Just sayin'.

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
File media/video/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.h:29: // Reallocates the video frame if
format or dimensions differ.
I think "differ" here refers to a mismatch between *video_frame and *av_frame. 
Can you clarify the comment and function name?
(if it's doing what I think it is then maybe AlignToAVFrame()?)

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.h:32: AVCodecContext* codec_context_;
Add ownership comments?

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
File media/video/video_decode_engine.h (left):

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:21: struct MEDIA_EXPORT EventHandler {
<SheddingHappyTears/>

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
File media/video/video_decode_engine.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:29: // Decode the compressed video data and
store the result (if any) into
s/compressed/encoded/
(here and below)

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:32: // additional calls to Decode() may be
required.
what if a buffer contains multiple frames?
(smells like maybe the client may need to detect this by "draining" Decode()
with empty buffers sometimes?  haven't looked at the rest of the CL yet).

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:34: // Returns true if operation was
successful, false if an error occurred.
More explicitly: false if decode error happens, true if no error happens?
(is EOS==false?  is no frame produced but no decode error == false?)

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:35: virtual bool Decode(scoped_refptr<Buffer>
buffer,
Should this be a const&?

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:38: // Flush the internal state of the engine.
Explicitly: drop pending encoded data that hasn't been returned as video_frames
yet on the floor?

(in particular this is different to VDA's Flush() which decodes all pending data
and delivers frames; what you have is closer to VDA's Reset())

[scherkus (not reviewing), 2011-11-03 04:55:59]

we're running into a lot of re-entrancy issues with our auto-trampolining checks

http://codereview.chromium.org/8417019/diff/8001/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/8001/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder.cc:58: frame_ready_cb_.Reset();  // is
this needed?
On 2011/11/01 18:31:03, acolwell wrote:
> I don't think this is needed. As far as I know Initialize() is only called
once
> at the beginning.

Done.

http://codereview.chromium.org/8417019/diff/8001/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder.cc:174:
base::TimeDelta::FromMilliseconds(30));
On 2011/11/01 18:31:03, acolwell wrote:
> Do we really want to hard code 30ms for the duration? Are we guaranteed that
we
> will always produce frames at 33.33fps or less?

That's a question for wjia to answer (will cc him when we're done with the bulk
of this review)

http://codereview.chromium.org/8417019/diff/8001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/base/filters.h#newcode176
media/base/filters.h:176: typedef
base::Callback<void(scoped_refptr<VideoFrame>)> FrameReadyCB;
On 2011/11/01 22:17:40, Ami Fischman wrote:
> On 2011/11/01 18:31:03, acolwell wrote:
> > Rename to ReadCB or ReadCallback? This is just to avoid confusion when
looking
> > at other code. read_cb_ is a little clearer than frame_ready_cb_ when trying
> to
> > evaluate where the callback came from.
> 
> FWIW, we decided a long time ago to use *CB for the new-style callbacks.  I
hope
> we keep that.

Renamed to ReadCB (I am fond of the shortness!) but left the bound callback
methods in VideoRendererBase and FFmpegVideoDecoderTest as FrameReady()

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:124: void
FFmpegVideoDecoder::DoDecodeBuffer(const scoped_refptr<Buffer>& buffer) {
On 2011/11/01 22:17:40, Ami Fischman wrote:
> Why have this be a separate function?  It makes DecodeBuffer() an empty shell.
> 
> (given the ugliness of having to trampoline explicitly, I prefer doing it
> in-line rather than having extra methods)

Done.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:153: //     Any time Flush() is called.
On 2011/11/01 22:17:40, Ami Fischman wrote:
> I'm surprised a Flush() can recover from a "catastrophic failure".  Perhaps
> downgrade that phrase to "decode error"?

Done.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:210: CHECK(!callback.is_null());
On 2011/11/01 22:17:40, Ami Fischman wrote:
> Here and below s/CHECK/DCHECK/?

maybe... I feel like this is programmer error and that the discovery of a null
read callback will be quite far from where it originates

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:217: // what?
On 2011/11/01 22:17:40, Ami Fischman wrote:
> I say drop the callback.

Done.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:244: // TODO(scherkus): change
DemuxerStream::Read() to use scoped_refptr<> for
On 2011/11/01 18:31:03, acolwell wrote:
> This TODO should be in demuxer_stream.h

TODO was fixed in r108178

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.h:36: virtual void Read(const FrameReadyCB&
callback); OVERRIDE;
On 2011/11/01 22:17:40, Ami Fischman wrote:
> This is part of some other CL, right?  Note that fact in the CL description?

Not sure what you mean -- this is replacing ProduceVideoFrame() with Read()?

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder_unittest.cc:320: // least one successful
decode but we don't expect ConsumeVideoFrame() to be
On 2011/11/01 18:31:03, acolwell wrote:
> Is this comment still accurate?

Done.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder_unittest.cc:326:
decoder_->Read(frame_ready_cb_);
On 2011/11/01 18:31:03, acolwell wrote:
> We just drop these calls? Is there any code in the renderer that waits on
reads
> to complete before flushing/pausing/stopping/seeking?

Good catch -- it indeed looks like VideoRendererBase would wait for a read to
complete during the pause->flush->stop shutdown path.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder_unittest.cc:356:
DecodeIFrameThenTestFile("vp8-I-frame-160x240", 160, 240);
On 2011/11/01 18:31:03, acolwell wrote:
> Frame size changes are actually working now?

They do as frames are always allocated to match whatever is output from FFmpeg.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder_unittest.cc:363: // 
DecodeIFrameThenTestFile("vp8-I-frame-320x480", 320, 480);
On 2011/11/01 18:31:03, acolwell wrote:
> If frame size changes are working now you may want to try uncommenting this
test
> and the one above to see if they work now.

Still failing due to bug in ffvp8 -- will changed to disabled and file a bug

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:79: if (thread_) {
On 2011/11/01 18:31:03, acolwell wrote:
> Given the code below shouldn't this be (thread_ != base::kNullThreadHandle) ?

Done.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:87: if (old_thread_handle)
On 2011/11/01 18:31:03, acolwell wrote:
> ditto

Done.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:374: CHECK(pending_read_);
On 2011/11/01 22:17:40, Ami Fischman wrote:
> DCHECK?

Again this one of those things where a callback was executed at a time when it
really shouldn't have -- I'm torn on whether shipping this with the CHECK() in

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:381: CHECK(false) << "I don't think this is
possible";
On 2011/11/01 22:17:40, Ami Fischman wrote:
> CHECK(false) is always better spelled LOG(FATAL)
> But if you really don't think so why not replace this clause & comment with
> DCHECK_NE(state_, kFlushed);
> ?
> 
> Ditto below.

Left these CHECK()s in for local testing -- they were never hit so will change
to DCHECK

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:386: if (state_ == kPaused) {
On 2011/11/01 18:31:03, acolwell wrote:
> Can't this happen based on the "Prerolling while paused" comment below? This
> looks like it will regress that feature.

I believe it can:
T1: VRB holds lock inside FrameReady()
T2: Pipeline calls Pause() but gets blocked on lock
T1: VRB requests another read and releases lock
T2: kState becomes pause
T1: VRB receives frame

I'll remove the check as it doesn't seem to cause much harm leaving it in

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:439: // when Seek() is eventually called.
On 2011/11/01 22:17:40, Ami Fischman wrote:
> English.

this and the comments up in Seek() are crazy bizarro

I don't believe they apply anymore as buffer recyling is gone

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:470: if (!pending_paint_ && !pending_read_)
{
On 2011/11/01 18:31:03, acolwell wrote:
> I think this may be where you can get into trouble if the decoder drops
> callbacks after a decode error. You'll never exit kFlushing if that happens.

Correct -- fixed + updated unit tests

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:506: void
VideoRendererBase::EnterErrorState_Locked(PipelineStatus status) {
On 2011/11/01 18:31:03, acolwell wrote:
> assuming you don't reinstate the call in FrameReady(), I think you can remove
> this method since the only other call sites are in Initialize(). 
> 
> There is enough code here that makes me think the FrameReady() call is pretty
> important.

Done.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:557: DCHECK(!pending_read_);
On 2011/11/01 18:31:03, acolwell wrote:
> Looking at the code this doesn't seem like a safe assumption especially if a
> decoder error occurs.

I really don't know what this method does but the two callsites either set
pending_read_ to false or guard against it being true before calling this.

Not sure what to do here -- perhaps inline the last_available_frame_ = NULL?

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.h:177: // before executing the flush callback;
We decrement it each time we receive
On 2011/11/01 22:17:40, Ami Fischman wrote:
> decrement/increment terminology is misleading in bool context.

Done.

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base_unittest.cc (left):

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base_unittest.cc:347: TEST_F(VideoRendererBaseTest,
Error_Playing) {
On 2011/11/01 22:17:40, Ami Fischman wrote:
> Why are these tests no longer needed?

because it's no longer legal to send VideoRendererBase a NULL frame

instead FFmpegVideoDecoder handles its own errors

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base_unittest.cc:277: VideoDecoder::FrameReadyCB
pending_read_;
On 2011/11/01 18:31:03, acolwell wrote:
> Rename to read_cb_?

Done.

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:132: //   2) End of stream was reached
and all internal frames have been output
On 2011/11/01 22:17:40, Ami Fischman wrote:
> I worry that these are things a client needs to know and thus belong in the
> public .h API, not here.

I thought I had that covered with the current documentation in the .h -- care to
suggest something more descriptive?

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
File media/video/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.h:29: // Reallocates the video frame if
format or dimensions differ.
On 2011/11/01 22:17:40, Ami Fischman wrote:
> I think "differ" here refers to a mismatch between *video_frame and *av_frame.

> Can you clarify the comment and function name?
> (if it's doing what I think it is then maybe AlignToAVFrame()?)

since we're no longer attempting to do buffer recycling in this CL we actually
are always allocating -- will update function name and docs

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.h:32: AVCodecContext* codec_context_;
On 2011/11/01 22:17:40, Ami Fischman wrote:
> Add ownership comments?

Done.

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
File media/video/video_decode_engine.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:29: // Decode the compressed video data and
store the result (if any) into
On 2011/11/01 22:17:40, Ami Fischman wrote:
> s/compressed/encoded/
> (here and below)

Done.

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:32: // additional calls to Decode() may be
required.
On 2011/11/01 22:17:40, Ami Fischman wrote:
> what if a buffer contains multiple frames?
> (smells like maybe the client may need to detect this by "draining" Decode()
> with empty buffers sometimes?  haven't looked at the rest of the CL yet).

Not sure how most of libavcodec handles that case. If the API contract is to
only deliver full frames at a time then that shouldn't happen. If the decoders
are resilient then I'd assume a loop around Decode() w/ empty buffers would be
required. AFAIK it's the former but haven't tested the latter.

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:34: // Returns true if operation was
successful, false if an error occurred.
On 2011/11/01 22:17:40, Ami Fischman wrote:
> More explicitly: false if decode error happens, true if no error happens?
> (is EOS==false?  is no frame produced but no decode error == false?)

Done.

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:35: virtual bool Decode(scoped_refptr<Buffer>
buffer,
On 2011/11/01 22:17:40, Ami Fischman wrote:
> Should this be a const&?

Done.

http://codereview.chromium.org/8417019/diff/8001/media/video/video_decode_eng...
media/video/video_decode_engine.h:38: // Flush the internal state of the engine.
On 2011/11/01 22:17:40, Ami Fischman wrote:
> Explicitly: drop pending encoded data that hasn't been returned as
video_frames
> yet on the floor?
> 
> (in particular this is different to VDA's Flush() which decodes all pending
data
> and delivers frames; what you have is closer to VDA's Reset())

Done.

http://codereview.chromium.org/8417019/diff/22001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/8417019/diff/22001/media/base/filters.h#newcod...
media/base/filters.h:177: // ^^^^^^^^^^^ it would be better to require callees
to post directly to
FYI

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:135: message_loop_->PostTask(FROM_HERE,
base::Bind(
FYI forced post task since VideoRendererBase::FrameReady() will call Read() on
FFmpegVideoDecoder's thread as FFVD executes read_cb_.

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:173: message_loop_->PostTask(FROM_HERE,
base::Bind(
FYI forced post task since FFmpegDemuxerStream::Read() will execute-and-return
on FFmpegVideoDecoder's thread

[Ami GONE FROM CHROMIUM, 2011-11-03 16:40:00]

LGTM modulo nits.  Leaving careful tests & state-machine review for acolwell.

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/8417019/diff/8001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.h:36: virtual void Read(const FrameReadyCB&
callback); OVERRIDE;
On 2011/11/03 04:55:59, scherkus wrote:
> On 2011/11/01 22:17:40, Ami Fischman wrote:
> > This is part of some other CL, right?  Note that fact in the CL description?
> 
> Not sure what you mean -- this is replacing ProduceVideoFrame() with Read()?

Now I'm not sure what I meant, either.  nm.

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/8417019/diff/8001/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:132: //   2) End of stream was reached
and all internal frames have been output
On 2011/11/03 04:55:59, scherkus wrote:
> On 2011/11/01 22:17:40, Ami Fischman wrote:
> > I worry that these are things a client needs to know and thus belong in the
> > public .h API, not here.
> 
> I thought I had that covered with the current documentation in the .h -- care
to
> suggest something more descriptive?

Before I was thinking you'd move this comment to the .h but current version
LGTM.

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:135: message_loop_->PostTask(FROM_HERE,
base::Bind(
On 2011/11/03 04:55:59, scherkus wrote:
> FYI forced post task since VideoRendererBase::FrameReady() will call Read() on
> FFmpegVideoDecoder's thread as FFVD executes read_cb_.

This is a better comment than what you have in the code.  Migrate?

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:139: gfx::Size
FFmpegVideoDecoder::natural_size() {
return const&?

http://codereview.chromium.org/8417019/diff/22001/media/filters/video_rendere...
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/8417019/diff/22001/media/filters/video_rendere...
media/filters/video_renderer_base.h:134: // favour of passing ownership of the
current frame to the renderer via
FWIW, IIUC, a notion of "last delivered frame" needs to be kept to support
webkit repaints when video frame isn't changing.  Possibly that can be pushed up
into webkit/glue so the pipeline only ever delivers a decoded frame once. 
That'd be nice.

http://codereview.chromium.org/8417019/diff/22001/media/video/ffmpeg_video_de...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/8417019/diff/22001/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine.cc:147: *video_frame =
AllocateVideoFrame();
return false if NULL?

[acolwell GONE FROM CHROMIUM, 2011-11-03 20:00:43]

LGTM just some nits on comments.

http://codereview.chromium.org/8417019/diff/22001/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/8417019/diff/22001/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:73: // Used for accessing |state_|
and |read_cb| from another thread.
read_cb_

state_ doesn't appear to be protected by this lock anywhere.

http://codereview.chromium.org/8417019/diff/22001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/8417019/diff/22001/media/base/filters.h#newcod...
media/base/filters.h:175: // This method must return immediately without
executing the callback.
Consider adding or replacing with "Implementations guarantee that the callback
will not be called from within this method."

We should also be explicit about whether it is ok for Read() to be called from
within the callback.

http://codereview.chromium.org/8417019/diff/22001/media/base/filters.h#newcod...
media/base/filters.h:177: // ^^^^^^^^^^^ it would be better to require callees
to post directly to
On 2011/11/03 04:55:59, scherkus wrote:
> FYI
I don't think this commentary is useful especially since it refers to
implementation details of the FFmpegVideoDecoder & VideoRendererBase
implementations.

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.h:56: // TODO(scherkus): mandate that
DemuxerStream::Read() execute asynchronously
Will making Read() async really remove the need for a post? If we did that
wouldn't it be likely the callback would then be called on the Demuxer thread
which would then require a post to the video decode thread? 

I'm fine with mandating Read() be asynchronous, but if doing so really isn't
going remove the need for a Post then I think this part of the comment should be
removed.

[scherkus (not reviewing), 2011-11-03 20:34:38]

thanks for the review!

wjia: can you sanity check the RTCVideoDecoder/CaptureVideoDecoder changes?

http://codereview.chromium.org/8417019/diff/22001/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/8417019/diff/22001/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:73: // Used for accessing |state_|
and |read_cb| from another thread.
On 2011/11/03 20:00:43, acolwell wrote:
> read_cb_
> 
> state_ doesn't appear to be protected by this lock anywhere.

Done.

http://codereview.chromium.org/8417019/diff/22001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/8417019/diff/22001/media/base/filters.h#newcod...
media/base/filters.h:175: // This method must return immediately without
executing the callback.
On 2011/11/03 20:00:43, acolwell wrote:
> Consider adding or replacing with "Implementations guarantee that the callback
> will not be called from within this method."
> 
> We should also be explicit about whether it is ok for Read() to be called from
> within the callback.

Done.

http://codereview.chromium.org/8417019/diff/22001/media/base/filters.h#newcod...
media/base/filters.h:177: // ^^^^^^^^^^^ it would be better to require callees
to post directly to
On 2011/11/03 20:00:43, acolwell wrote:
> On 2011/11/03 04:55:59, scherkus wrote:
> > FYI
> I don't think this commentary is useful especially since it refers to
> implementation details of the FFmpegVideoDecoder & VideoRendererBase
> implementations.
> 

Oh I wasn't planning on checking this comment in!  It was for you reviewers to
look at and comment on (I guess it worked!)

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:135: message_loop_->PostTask(FROM_HERE,
base::Bind(
On 2011/11/03 16:40:00, Ami Fischman wrote:
> On 2011/11/03 04:55:59, scherkus wrote:
> > FYI forced post task since VideoRendererBase::FrameReady() will call Read()
on
> > FFmpegVideoDecoder's thread as FFVD executes read_cb_.
> 
> This is a better comment than what you have in the code.  Migrate?

Done.

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:139: gfx::Size
FFmpegVideoDecoder::natural_size() {
On 2011/11/03 16:40:00, Ami Fischman wrote:
> return const&?

make ALL the return types const&!

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:173: message_loop_->PostTask(FROM_HERE,
base::Bind(
On 2011/11/03 04:55:59, scherkus wrote:
> FYI forced post task since FFmpegDemuxerStream::Read() will execute-and-return
> on FFmpegVideoDecoder's thread

Comment added.

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.h:56: // TODO(scherkus): mandate that
DemuxerStream::Read() execute asynchronously
On 2011/11/03 20:00:43, acolwell wrote:
> Will making Read() async really remove the need for a post? If we did that
> wouldn't it be likely the callback would then be called on the Demuxer thread
> which would then require a post to the video decode thread? 
> 
> I'm fine with mandating Read() be asynchronous, but if doing so really isn't
> going remove the need for a Post then I think this part of the comment should
be
> removed.

This TODO has been replaced w/ the TODO inside the .cc

http://codereview.chromium.org/8417019/diff/22001/media/filters/video_rendere...
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/8417019/diff/22001/media/filters/video_rendere...
media/filters/video_renderer_base.h:134: // favour of passing ownership of the
current frame to the renderer via
On 2011/11/03 16:40:00, Ami Fischman wrote:
> FWIW, IIUC, a notion of "last delivered frame" needs to be kept to support
> webkit repaints when video frame isn't changing.  Possibly that can be pushed
up
> into webkit/glue so the pipeline only ever delivers a decoded frame once. 
> That'd be nice.

Yes the only reason why we have last_available_frame_ is due to wonky buffer
recycling. We can remove in a follow up CL.

http://codereview.chromium.org/8417019/diff/22001/media/video/ffmpeg_video_de...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/8417019/diff/22001/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine.cc:147: *video_frame =
AllocateVideoFrame();
On 2011/11/03 16:40:00, Ami Fischman wrote:
> return false if NULL?

Done.

[wjia(left Chromium), 2011-11-04 00:16:06]

Video capture preview crashes. The reason is that natural_size_ is not set
correctly. Attached are modified capture_video_decoder.h,
capture_video_decoder.cc, and capture_video_decoder_unittest.cc.

To test video capture preview, run "chrome --enable-media-stream
www.corp.<http://www.corp.google.com/~wjia/preview.html>
google.com/~wjia/preview.html<http://www.corp.google.com/~wjia/preview.html>"
on corp network.

Wei

On Thu, Nov 3, 2011 at 1:34 PM, <scherkus@chromium.org> wrote:

> thanks for the review!
>
> wjia: can you sanity check the RTCVideoDecoder/**CaptureVideoDecoder
> changes?
>
>
> http://codereview.chromium.**org/8417019/diff/22001/**
>
content/renderer/media/rtc_**video_decoder.h<http://codereview.chromium.org/8417019/diff/22001/content/renderer/media/rtc_video_decoder.h>
> File content/renderer/media/rtc_**video_decoder.h (right):
>
> http://codereview.chromium.**org/8417019/diff/22001/**
>
content/renderer/media/rtc_**video_decoder.h#newcode73<http://codereview.chromium.org/8417019/diff/22001/content/renderer/media/rtc_video_decoder.h#newcode73>
> content/renderer/media/rtc_**video_decoder.h:73: // Used for accessing
> |state_| and |read_cb| from another thread.
> On 2011/11/03 20:00:43, acolwell wrote:
>
>> read_cb_
>>
>
>  state_ doesn't appear to be protected by this lock anywhere.
>>
>
> Done.
>
>
http://codereview.chromium.**org/8417019/diff/22001/media/**base/filters.h<ht...
> File media/base/filters.h (right):
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
base/filters.h#newcode175<http://codereview.chromium.org/8417019/diff/22001/media/base/filters.h#newcode175>
> media/base/filters.h:175: // This method must return immediately without
> executing the callback.
> On 2011/11/03 20:00:43, acolwell wrote:
>
>> Consider adding or replacing with "Implementations guarantee that the
>>
> callback
>
>> will not be called from within this method."
>>
>
>  We should also be explicit about whether it is ok for Read() to be
>>
> called from
>
>> within the callback.
>>
>
> Done.
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
base/filters.h#newcode177<http://codereview.chromium.org/8417019/diff/22001/media/base/filters.h#newcode177>
> media/base/filters.h:177: // ^^^^^^^^^^^ it would be better to require
> callees to post directly to
> On 2011/11/03 20:00:43, acolwell wrote:
>
>> On 2011/11/03 04:55:59, scherkus wrote:
>> > FYI
>> I don't think this commentary is useful especially since it refers to
>> implementation details of the FFmpegVideoDecoder & VideoRendererBase
>> implementations.
>>
>
>
> Oh I wasn't planning on checking this comment in!  It was for you
> reviewers to look at and comment on (I guess it worked!)
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
filters/ffmpeg_video_decoder.**cc<http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_decoder.cc>
> File media/filters/ffmpeg_video_**decoder.cc (right):
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
filters/ffmpeg_video_decoder.**cc#newcode135<http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_decoder.cc#newcode135>
> media/filters/ffmpeg_video_**decoder.cc:135:
> message_loop_->PostTask(FROM_**HERE, base::Bind(
> On 2011/11/03 16:40:00, Ami Fischman wrote:
>
>> On 2011/11/03 04:55:59, scherkus wrote:
>> > FYI forced post task since VideoRendererBase::FrameReady(**) will call
>>
> Read() on
>
>> > FFmpegVideoDecoder's thread as FFVD executes read_cb_.
>>
>
>  This is a better comment than what you have in the code.  Migrate?
>>
>
> Done.
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
filters/ffmpeg_video_decoder.**cc#newcode139<http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_decoder.cc#newcode139>
> media/filters/ffmpeg_video_**decoder.cc:139: gfx::Size
> FFmpegVideoDecoder::natural_**size() {
> On 2011/11/03 16:40:00, Ami Fischman wrote:
>
>> return const&?
>>
>
> make ALL the return types const&!
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
filters/ffmpeg_video_decoder.**cc#newcode173<http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_decoder.cc#newcode173>
> media/filters/ffmpeg_video_**decoder.cc:173:
> message_loop_->PostTask(FROM_**HERE, base::Bind(
> On 2011/11/03 04:55:59, scherkus wrote:
>
>> FYI forced post task since FFmpegDemuxerStream::Read() will
>>
> execute-and-return
>
>> on FFmpegVideoDecoder's thread
>>
>
> Comment added.
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
filters/ffmpeg_video_decoder.h<http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_decoder.h>
> File media/filters/ffmpeg_video_**decoder.h (right):
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
filters/ffmpeg_video_decoder.**h#newcode56<http://codereview.chromium.org/8417019/diff/22001/media/filters/ffmpeg_video_decoder.h#newcode56>
> media/filters/ffmpeg_video_**decoder.h:56: // TODO(scherkus): mandate that
> DemuxerStream::Read() execute asynchronously
> On 2011/11/03 20:00:43, acolwell wrote:
>
>> Will making Read() async really remove the need for a post? If we did
>>
> that
>
>> wouldn't it be likely the callback would then be called on the Demuxer
>>
> thread
>
>> which would then require a post to the video decode thread?
>>
>
>  I'm fine with mandating Read() be asynchronous, but if doing so really
>>
> isn't
>
>> going remove the need for a Post then I think this part of the comment
>>
> should be
>
>> removed.
>>
>
> This TODO has been replaced w/ the TODO inside the .cc
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
filters/video_renderer_base.h<http://codereview.chromium.org/8417019/diff/22001/media/filters/video_renderer_base.h>
> File media/filters/video_renderer_**base.h (right):
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
filters/video_renderer_base.h#**newcode134<http://codereview.chromium.org/8417019/diff/22001/media/filters/video_renderer_base.h#newcode134>
> media/filters/video_renderer_**base.h:134: // favour of passing ownership
> of the current frame to the renderer via
> On 2011/11/03 16:40:00, Ami Fischman wrote:
>
>> FWIW, IIUC, a notion of "last delivered frame" needs to be kept to
>>
> support
>
>> webkit repaints when video frame isn't changing.  Possibly that can be
>>
> pushed up
>
>> into webkit/glue so the pipeline only ever delivers a decoded frame
>>
> once.
>
>> That'd be nice.
>>
>
> Yes the only reason why we have last_available_frame_ is due to wonky
> buffer recycling. We can remove in a follow up CL.
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
video/ffmpeg_video_decode_**engine.cc<http://codereview.chromium.org/8417019/diff/22001/media/video/ffmpeg_video_decode_engine.cc>
> File media/video/ffmpeg_video_**decode_engine.cc (right):
>
> http://codereview.chromium.**org/8417019/diff/22001/media/**
>
video/ffmpeg_video_decode_**engine.cc#newcode147<http://codereview.chromium.org/8417019/diff/22001/media/video/ffmpeg_video_decode_engine.cc#newcode147>
> media/video/ffmpeg_video_**decode_engine.cc:147: *video_frame =
> AllocateVideoFrame();
> On 2011/11/03 16:40:00, Ami Fischman wrote:
>
>> return false if NULL?
>>
>
> Done.
>
>
http://codereview.chromium.**org/8417019/<http://codereview.chromium.org/8417...
>

[scherkus (not reviewing), 2011-11-04 00:37:38]

ah! you must have tried PS#7 instead of PS#8 which has the fix

I tried out your link with PS#8 and it works great (cool demo!)

[wjia(left Chromium), 2011-11-04 00:43:28]

Please use the fix I have attached. The PS#8 happens to work for some cases
where requested resolutions are supported by camera. Otherwise, it will
have problem. We need to use resolution returned by camera, not the one
requested by user.

It's ok if you want to make a follow up patch.

Wei

On Thu, Nov 3, 2011 at 5:37 PM, <scherkus@chromium.org> wrote:

> ah! you must have tried PS#7 instead of PS#8 which has the fix
>
> I tried out your link with PS#8 and it works great (cool demo!)
>
>
http://codereview.chromium.**org/8417019/<http://codereview.chromium.org/8417...
>

[scherkus (not reviewing), 2011-11-04 02:47:47]

On 2011/11/04 00:43:28, wjia wrote:
> Please use the fix I have attached. The PS#8 happens to work for some cases
> where requested resolutions are supported by camera. Otherwise, it will
> have problem. We need to use resolution returned by camera, not the one
> requested by user.
> 
> It's ok if you want to make a follow up patch.

Followup patch here:
http://codereview.chromium.org/8466001
 
> On Thu, Nov 3, 2011 at 5:37 PM, <mailto:scherkus@chromium.org> wrote:
> 
> > ah! you must have tried PS#7 instead of PS#8 which has the fix
> >
> > I tried out your link with PS#8 and it works great (cool demo!)
> >
> >
>
http://codereview.chromium.**org/8417019/%3Chttp://codereview.chromium.org/84...>
> >