|
|
Created:
11 years, 10 months ago by ralphl Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAlmost complete implementation of the Chrome video renderer. Still needs to implement color space conversion for final bitblt.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9575
Patch Set 1 #Patch Set 2 : '' #
Total comments: 48
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 19
Patch Set 5 : '' #
Messages
Total messages: 8 (0 generated)
Here's the (almost) complete video renderer. This change relies on the previous change that I submitted that supports FilterHost::ScheduleTimeUpateCallback.
I haven't got to the meat of the CL and I have to go soon but I have some comments. http://codereview.chromium.org/21037/diff/1001/10 File chrome/renderer/media/video_renderer_impl.h (right): http://codereview.chromium.org/21037/diff/1001/10#newcode24 Line 24: #include "base/gfx/platform_canvas.h" out of order includes http://codereview.chromium.org/21037/diff/1001/10#newcode40 Line 40: // video? typo Streatching http://codereview.chromium.org/21037/diff/1001/10#newcode61 Line 61: class SubmitReadsTask : public CancelableTask { can this class move to the .cc ? I don't see the gain for it to be in this header or being an inner class. http://codereview.chromium.org/21037/diff/1001/10#newcode104 Line 104: bool IsRunning() { return (delegate_ != NULL); } const http://codereview.chromium.org/21037/diff/1001/10#newcode110 Line 110: // Given the current |time|, this method updates the state of the video frame Is there a risk this method will be called outside the lock? I mention it because this is private method on an impl class, one will expect that preconditions will be guaranteed by the parent or by the public method that ends up calling this. http://codereview.chromium.org/21037/diff/1001/10#newcode118 Line 118: // CALLING FUNCTION MUST CALL |front_frame_out|->Release(); I am somewhat confused with the 'WILL HAVE BEEN' .. you mean the reference count will be incremented by the function regardless of the return value? by the way the return value false means failure? http://codereview.chromium.org/21037/diff/1001/10#newcode164 Line 164: // |current_frame_| member contains the correct image for the queue front. Why is the timestamp indicating the colorspace? just curious... http://codereview.chromium.org/21037/diff/1001/10#newcode182 Line 182: static const int64 kFrameSkipAheadLimit = 5000; can all this static constants move to the .cc ? they seem to be private and their modification will cause needless recompile of other code. In the cc they should go into an unnamed namespace. http://codereview.chromium.org/21037/diff/1001/10#newcode186 Line 186: static const int64 kSleepIfNoFrame = 20000; In ms or in us ?
few comments http://codereview.chromium.org/21037/diff/1001/9 File chrome/renderer/media/video_renderer_impl.cc (right): http://codereview.chromium.org/21037/diff/1001/9#newcode8 Line 8: #include "media/base/buffers.h" alphabetize includes http://codereview.chromium.org/21037/diff/1001/9#newcode22 Line 22: DiscardAllFrames(); should you acquire the queue lock here? http://codereview.chromium.org/21037/diff/1001/9#newcode27 Line 27: const media::MediaFormat* media_format) { 4 spaces for indent http://codereview.chromium.org/21037/diff/1001/9#newcode84 Line 84: media::VideoFrame>(this)); align parameters http://codereview.chromium.org/21037/diff/1001/9#newcode116 Line 116: if (new_frame) { add short comments explaining what these chunks of code are doing http://codereview.chromium.org/21037/diff/1001/9#newcode162 Line 162: void VideoRendererImpl::DiscardAllFrames() { you should either have this method acquire the lock or add a comment that it expects the lock has been acquired http://codereview.chromium.org/21037/diff/1001/9#newcode188 Line 188: if (false) { // TODO(ralph): If(SeekFrame()) then discard.... could you move the comment to before the if statement http://codereview.chromium.org/21037/diff/1001/9#newcode202 Line 202: // TODO(ralph): actually do something here.... remove this whole bit of commented out code http://codereview.chromium.org/21037/diff/1001/9#newcode238 Line 238: /* get rid of this big block of commented out code and instead add a nice TODO message to perform colour space conversion http://codereview.chromium.org/21037/diff/1001/9#newcode267 Line 267: // TODO(ralphl): I have no idea what's going on here. How are these SetRect is trickled down from RenderVideo to MediaPlayer to this class. It gives you the offset and scaled video size you are supposed to render to (maintains video aspect ratio). The rect from paint is actually the exact same rect, but with a potential extra transform (not sure why). I'm actually confused now because this code works but I feel like rect and rect_ should be equal, meaning we aren't scaling. http://codereview.chromium.org/21037/diff/1001/10 File chrome/renderer/media/video_renderer_impl.h (right): http://codereview.chromium.org/21037/diff/1001/10#newcode43 Line 43: virtual void Paint(skia::PlatformCanvas *canvas, const gfx::Rect& rect); pointer with type http://codereview.chromium.org/21037/diff/1001/10#newcode150 Line 150: Lock lock_; if this lock is only used with video frame queue, then I would name it something like queue_lock_
http://codereview.chromium.org/21037/diff/1001/9 File chrome/renderer/media/video_renderer_impl.cc (right): http://codereview.chromium.org/21037/diff/1001/9#newcode18 Line 18: base::TimeDelta::FromMicroseconds(kNoCurrentFrame)) { too much indent http://codereview.chromium.org/21037/diff/1001/9#newcode77 Line 77: lock_.Acquire(); Suggestion use AutoLock l(&lock_) and a block. http://codereview.chromium.org/21037/diff/1001/9#newcode124 Line 124: updated_front = (iter == queue_.begin()); I am somewhat lost about this logic. Afaik, iterators of deques are invalidated after an insert. Not 100% sure though.
I've addressed all of the comments by Carlos and Andrew. I cleaned up the Paint() logic so I think it is much more obvious what is happening, and consolidated the color space conversion into it's own method. This change is blocked on the Interpolated Time change, so please review that first, so I can get it checked in. http://codereview.chromium.org/21037/diff/1001/9 File chrome/renderer/media/video_renderer_impl.cc (right): http://codereview.chromium.org/21037/diff/1001/9#newcode8 Line 8: #include "media/base/buffers.h" On 2009/02/06 00:59:41, scherkus wrote: > alphabetize includes Done. http://codereview.chromium.org/21037/diff/1001/9#newcode18 Line 18: base::TimeDelta::FromMicroseconds(kNoCurrentFrame)) { On 2009/02/06 21:33:30, cpu wrote: > too much indent Done. http://codereview.chromium.org/21037/diff/1001/9#newcode22 Line 22: DiscardAllFrames(); On 2009/02/06 00:59:41, scherkus wrote: > should you acquire the queue lock here? Good catch. Yes the lock needs to be taken here. http://codereview.chromium.org/21037/diff/1001/9#newcode27 Line 27: const media::MediaFormat* media_format) { On 2009/02/06 00:59:41, scherkus wrote: > 4 spaces for indent Done. http://codereview.chromium.org/21037/diff/1001/9#newcode77 Line 77: lock_.Acquire(); On 2009/02/06 21:33:30, cpu wrote: > Suggestion use AutoLock l(&lock_) and a block. Done. http://codereview.chromium.org/21037/diff/1001/9#newcode84 Line 84: media::VideoFrame>(this)); On 2009/02/06 00:59:41, scherkus wrote: > align parameters Done. http://codereview.chromium.org/21037/diff/1001/9#newcode116 Line 116: if (new_frame) { On 2009/02/06 00:59:41, scherkus wrote: > add short comments explaining what these chunks of code are doing Done. http://codereview.chromium.org/21037/diff/1001/9#newcode124 Line 124: updated_front = (iter == queue_.begin()); On 2009/02/06 21:33:30, cpu wrote: > I am somewhat lost about this logic. Afaik, iterators of deques are invalidated > after an insert. Not 100% sure though. > > Added a comment plus I moved the updated_front comparison up to before I insert something in the queue. The function wants to know if the front of the queue was updated, because that means that we need to paint a new frame. http://codereview.chromium.org/21037/diff/1001/9#newcode162 Line 162: void VideoRendererImpl::DiscardAllFrames() { On 2009/02/06 00:59:41, scherkus wrote: > you should either have this method acquire the lock or add a comment that it > expects the lock has been acquired Done. http://codereview.chromium.org/21037/diff/1001/9#newcode188 Line 188: if (false) { // TODO(ralph): If(SeekFrame()) then discard.... On 2009/02/06 00:59:41, scherkus wrote: > could you move the comment to before the if statement Done. http://codereview.chromium.org/21037/diff/1001/9#newcode202 Line 202: // TODO(ralph): actually do something here.... On 2009/02/06 00:59:41, scherkus wrote: > remove this whole bit of commented out code Done. http://codereview.chromium.org/21037/diff/1001/9#newcode238 Line 238: /* On 2009/02/06 00:59:41, scherkus wrote: > get rid of this big block of commented out code and instead add a nice TODO > message to perform colour space conversion Done. http://codereview.chromium.org/21037/diff/1001/9#newcode267 Line 267: // TODO(ralphl): I have no idea what's going on here. How are these On 2009/02/06 00:59:41, scherkus wrote: > SetRect is trickled down from RenderVideo to MediaPlayer to this class. It > gives you the offset and scaled video size you are supposed to render to > (maintains video aspect ratio). > > The rect from paint is actually the exact same rect, but with a potential extra > transform (not sure why). I'm actually confused now because this code works but > I feel like rect and rect_ should be equal, meaning we aren't scaling. So I still don't understand it completely, so I've left the todo()'s until I do understand it and can document what they are. http://codereview.chromium.org/21037/diff/1001/10 File chrome/renderer/media/video_renderer_impl.h (right): http://codereview.chromium.org/21037/diff/1001/10#newcode24 Line 24: #include "base/gfx/platform_canvas.h" On 2009/02/05 02:10:55, cpu wrote: > out of order includes Done. http://codereview.chromium.org/21037/diff/1001/10#newcode40 Line 40: // video? On 2009/02/05 02:10:55, cpu wrote: > typo Streatching Done. http://codereview.chromium.org/21037/diff/1001/10#newcode43 Line 43: virtual void Paint(skia::PlatformCanvas *canvas, const gfx::Rect& rect); On 2009/02/06 00:59:41, scherkus wrote: > pointer with type Done. http://codereview.chromium.org/21037/diff/1001/10#newcode61 Line 61: class SubmitReadsTask : public CancelableTask { On 2009/02/05 02:10:55, cpu wrote: > can this class move to the .cc ? I don't see the gain for it to be in this > header or being an inner class. Done. http://codereview.chromium.org/21037/diff/1001/10#newcode104 Line 104: bool IsRunning() { return (delegate_ != NULL); } On 2009/02/05 02:10:55, cpu wrote: > const Done. http://codereview.chromium.org/21037/diff/1001/10#newcode110 Line 110: // Given the current |time|, this method updates the state of the video frame On 2009/02/05 02:10:55, cpu wrote: > Is there a risk this method will be called outside the lock? I mention it > because this is private method on an impl class, one will expect that > preconditions will be guaranteed by the parent or by the public method that ends > up calling this. The comment in ALL CAPS is intended for anyone who attempts to modify this class in the future. It is always called with |lock_| acquired in this implementation. http://codereview.chromium.org/21037/diff/1001/10#newcode118 Line 118: // CALLING FUNCTION MUST CALL |front_frame_out|->Release(); On 2009/02/05 02:10:55, cpu wrote: > I am somewhat confused with the 'WILL HAVE BEEN' .. you mean the reference count > will be incremented by the function regardless of the return value? by the way > the return value false means failure? I updated the comments to clarify. Please see if it's more clear. I agree with your comments -- it was not clear. http://codereview.chromium.org/21037/diff/1001/10#newcode150 Line 150: Lock lock_; On 2009/02/06 00:59:41, scherkus wrote: > if this lock is only used with video frame queue, then I would name it something > like queue_lock_ It's used for protecting members like the submit_reads_task_ and the current_frame_timestamp_ members too so I think just plain old lock_ is probably best. http://codereview.chromium.org/21037/diff/1001/10#newcode164 Line 164: // |current_frame_| member contains the correct image for the queue front. On 2009/02/05 02:10:55, cpu wrote: > Why is the timestamp indicating the colorspace? just curious... We convert from YUV to RGB. Once a frame has been converted it can be drawn to the render process in the Paint method. If we are called multiple times for the same frame, we don't want to do the YUV to RGB conversion. We can just draw out of the current_frame_ member http://codereview.chromium.org/21037/diff/1001/10#newcode182 Line 182: static const int64 kFrameSkipAheadLimit = 5000; On 2009/02/05 02:10:55, cpu wrote: > can all this static constants move to the .cc ? they seem to be private and > their modification will cause needless recompile of other code. > > In the cc they should go into an unnamed namespace. Done. http://codereview.chromium.org/21037/diff/1001/10#newcode186 Line 186: static const int64 kSleepIfNoFrame = 20000; On 2009/02/05 02:10:55, cpu wrote: > In ms or in us ? Fixed comment. In microseconds.
Some additional comments. http://codereview.chromium.org/21037/diff/15/16 File chrome/renderer/media/video_renderer_impl.cc (right): http://codereview.chromium.org/21037/diff/15/16#newcode10 Line 10: using media::MediaFormat; not sure if its cool to use usings, will have to check http://codereview.chromium.org/21037/diff/15/16#newcode42 Line 42: DCHECK(media_format && width_out && height_out); could you split up the DCHECK? http://codereview.chromium.org/21037/diff/15/16#newcode83 Line 83: host_->InitializationComplete(); I'm thinking we should consider ourselves initialized after we receive our first read and do our first paint, otherwise we haven't exactly finished preroll. http://codereview.chromium.org/21037/diff/15/16#newcode110 Line 110: void VideoRendererImpl::Paint(skia::PlatformCanvas *canvas, so are we lazy-converting to RGB as opposed to doing it deterministically (say, inside OnAssignment)? just curious at the idea http://codereview.chromium.org/21037/diff/15/16#newcode171 Line 171: bool VideoRendererImpl::UpdateQueue(base::TimeDelta time, Looking at the callees of UpdateQueue, it looks like you can move the lock_ acquire inside the function. I can't find any place where UpdateQueue and another action has to happen atomically. http://codereview.chromium.org/21037/diff/15/16#newcode180 Line 180: if (new_frame) { Out of curiosity, why would the frames be passed in out-of-order? Decoders take care of this to ensure they only deliver samples in increasing presentation timestamp order. If anything it should be a DCHECK :) http://codereview.chromium.org/21037/diff/15/16#newcode245 Line 245: void VideoRendererImpl::DiscardAllFrames() { Similarly, I can't find any callee where DiscardAllFrames and another actions needs to happen atomically. You can consider moving the lock acquire into here. http://codereview.chromium.org/21037/diff/15/16#newcode288 Line 288: SubmitReadsTask::SubmitReadsTask(VideoRendererImpl* video_renderer) You can just use NewRunnableMethod(this, &VideoRendererImpl::SubmitReads) since it actually already is a CancelableTask and implements this exact logic. http://codereview.chromium.org/21037/diff/15/17 File chrome/renderer/media/video_renderer_impl.h (right): http://codereview.chromium.org/21037/diff/15/17#newcode20 Line 20: #include "base/lock.h" alphabetize includes http://codereview.chromium.org/21037/diff/15/17#newcode166 Line 166: static const int64 kFrameSkipAheadLimit; Unless anyone else is going to call them, you might as well not make them part of this class and declare them in the cc.
Incorporated Andrew's comments, and documented the various things the |lock_| is protecting. http://codereview.chromium.org/21037/diff/15/16 File chrome/renderer/media/video_renderer_impl.cc (right): http://codereview.chromium.org/21037/diff/15/16#newcode42 Line 42: DCHECK(media_format && width_out && height_out); On 2009/02/10 23:29:08, scherkus wrote: > could you split up the DCHECK? Carlos told me to collapse it. I'm leaving it alone. http://codereview.chromium.org/21037/diff/15/16#newcode83 Line 83: host_->InitializationComplete(); On 2009/02/10 23:29:08, scherkus wrote: > I'm thinking we should consider ourselves initialized after we receive our first > read and do our first paint, otherwise we haven't exactly finished preroll. I agree. I've added a new member and call this in the OnAssign member function. http://codereview.chromium.org/21037/diff/15/16#newcode110 Line 110: void VideoRendererImpl::Paint(skia::PlatformCanvas *canvas, On 2009/02/10 23:29:08, scherkus wrote: > so are we lazy-converting to RGB as opposed to doing it deterministically (say, > inside OnAssignment)? > > just curious at the idea We don't want to convert frames we never show (for example, we're in a tab that is obscured) and we don't want to color convert a frame multiple times if, for example, we're at rate=0.0f and we keep repainting the same frame. http://codereview.chromium.org/21037/diff/15/16#newcode171 Line 171: bool VideoRendererImpl::UpdateQueue(base::TimeDelta time, On 2009/02/10 23:29:08, scherkus wrote: > Looking at the callees of UpdateQueue, it looks like you can move the lock_ > acquire inside the function. I can't find any place where UpdateQueue and > another action has to happen atomically. See the new, extensive comment on the |lock_| member. http://codereview.chromium.org/21037/diff/15/16#newcode180 Line 180: if (new_frame) { On 2009/02/10 23:29:08, scherkus wrote: > Out of curiosity, why would the frames be passed in out-of-order? Decoders take > care of this to ensure they only deliver samples in increasing presentation > timestamp order. > > If anything it should be a DCHECK :) I just stick it at the end of the queue now. http://codereview.chromium.org/21037/diff/15/16#newcode245 Line 245: void VideoRendererImpl::DiscardAllFrames() { On 2009/02/10 23:29:08, scherkus wrote: > Similarly, I can't find any callee where DiscardAllFrames and another actions > needs to happen atomically. You can consider moving the lock acquire into here. See comment on |lock_| http://codereview.chromium.org/21037/diff/15/16#newcode288 Line 288: SubmitReadsTask::SubmitReadsTask(VideoRendererImpl* video_renderer) On 2009/02/10 23:29:08, scherkus wrote: > You can just use NewRunnableMethod(this, &VideoRendererImpl::SubmitReads) since > it actually already is a CancelableTask and implements this exact logic. Very nice! Removed the entire thing. http://codereview.chromium.org/21037/diff/15/17 File chrome/renderer/media/video_renderer_impl.h (right): http://codereview.chromium.org/21037/diff/15/17#newcode20 Line 20: #include "base/lock.h" On 2009/02/10 23:29:08, scherkus wrote: > alphabetize includes Done. http://codereview.chromium.org/21037/diff/15/17#newcode166 Line 166: static const int64 kFrameSkipAheadLimit; On 2009/02/10 23:29:08, scherkus wrote: > Unless anyone else is going to call them, you might as well not make them part > of this class and declare them in the cc. Done.
LGTM any one else have reviews? |