Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
462413004:
rendering_helper - Refactoring - remove the Client interface. (Closed)
|
Created:
6 years, 4 months ago by Owen Lin Modified:
6 years, 4 months ago Reviewers:
Pawel Osciak CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionrendering_helper - Refactoring - remove the Client interface.
Use a callback to return the released picture from rendering_helper. So we don't
need the RenderingHelper::Client interface and thus simplify the code.
BUG=None
TEST=Run the vda_unittest on lumpy and ARM CrOS device.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291061
Patch Set 1 #
Total comments: 46
Patch Set 2 : address review comments #
Total comments: 21
Patch Set 3 : #
Total comments: 14
Patch Set 4 : address review comment #Patch Set 5 : fix compiling errors(warnings) #Patch Set 6 : fix one more compiling error #
Messages
Total messages: 18 (0 generated)
PTAL.
Beautiful change, so much simpler than before. Thanks Owen. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:74: if (!no_longer_needed_cb_.is_null()) I don't think we can allow this to be optional, better to check it's not null in constructor? https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:172: clients_.resize(params.window_sizes.size()); This should go to LayoutRenderingAreas() maybe? https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:394: delete client->pending_frames.front(); Please use container of scoped_refptrs and just pop. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:399: client->last_frame_rendered = false; Technically this should go to the if clause? https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:403: STLDeleteElements(&clients_[window_id].pending_frames); client->last_frame_rendered = false just in case? https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:517: // Releases the rendered frame after SwapBuffers() if it is not the last one. Instead of this, just check if client->pending_frames.size() > 1 in the first loop and if yes, pop it from there into a scoped_ptr, then let it drop out of scope after this method returns. That way you can also take swapbuffers out of the render thumbnails loop and put it at the end of the method: RenderContent() { std::vector<scoped_refptr> frames_to_be_returned; if (render_as_thumbnails_) { // render } else { // render if (client->pending_frames.size() > 1) { frames_to_be_returned.push_back(client->pending_frames.front()); client->pending_frames.pop_front(); client->last_frame_rendered = false; } } SwapBuffers(); } https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:28: class VideoFrame { Why not use media::VideoFrame? https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:51: // The designed size of each window. s/designed// Please explain this is used for the regular playback mode with each stream playing in its own window on the screen, and the members below that they are used for the thumbnail mode where all frames are rendered in sequence onto one FBO for comparison/verification purposes in thumbnail test. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:70: Not needed. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:96: void ClearVideoFrames(size_t window_id); Please document and rename to DropPendingFrames(). https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:114: struct RenderingClient { I think we should not call it a Client, because this is not a Client, just a struct with data for clients. Otherwise it would imply that we owned the clients by having a vector of them below. How about RenderedVideo ? https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:115: gfx::Rect render_area; Please documentation for all members. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:116: bool last_frame_rendered; Isn't this equivalent to pending_frames.empty() ? https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:118: std::deque<VideoFrame*> pending_frames; Please keep scoped_refptrs here. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:120: inline RenderingClient() : last_frame_rendered(false) {} s/inline// https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:131: Remove empty line please. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:441: new media::PictureBuffer(id, dimensions, texture_id); Could we use linked_ptr here instead of new/delete? https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:467: frame_delivery_times_.push_back(now); Yay! https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:1380: CommandLine::Init(argc, argv); Please don't remove base:: https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:1387: CommandLine* cmd_line = CommandLine::ForCurrentProcess(); nit: const
Oh and please also test on ARM of course.
Thanks for the comments. PTAL. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:74: if (!no_longer_needed_cb_.is_null()) On 2014/08/14 04:48:23, Pawel Osciak wrote: > I don't think we can allow this to be optional, better to check it's not null in > constructor? Done. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:172: clients_.resize(params.window_sizes.size()); On 2014/08/14 04:48:22, Pawel Osciak wrote: > This should go to LayoutRenderingAreas() maybe? mmm... I would prefer keep it here. As the name suggesting, it only do the layout. We may need other initialization for all the clients. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:394: delete client->pending_frames.front(); On 2014/08/14 04:48:23, Pawel Osciak wrote: > Please use container of scoped_refptrs and just pop. Done https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:399: client->last_frame_rendered = false; On 2014/08/14 04:48:23, Pawel Osciak wrote: > Technically this should go to the if clause? Yes, the code is modified. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:403: STLDeleteElements(&clients_[window_id].pending_frames); On 2014/08/14 04:48:23, Pawel Osciak wrote: > client->last_frame_rendered = false just in case? Good catch. Thanks. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:517: // Releases the rendered frame after SwapBuffers() if it is not the last one. Done. I was avoid using RefCounted object since the VF actually has one owner. But it did simplify the code a lot. On 2014/08/14 04:48:22, Pawel Osciak wrote: > Instead of this, just check if client->pending_frames.size() > 1 in the first > loop and if yes, pop it from there into a scoped_ptr, then let it drop out of > scope after this method returns. > > That way you can also take swapbuffers out of the render thumbnails loop and put > it at the end of the method: > > > RenderContent() { > std::vector<scoped_refptr> frames_to_be_returned; > if (render_as_thumbnails_) { > // render > } else { > // render > if (client->pending_frames.size() > 1) { > frames_to_be_returned.push_back(client->pending_frames.front()); > client->pending_frames.pop_front(); > client->last_frame_rendered = false; > } > } > > SwapBuffers(); > } https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:28: class VideoFrame { On 2014/08/14 04:48:23, Pawel Osciak wrote: > Why not use media::VideoFrame? The main reason is I don't know how to use it in our case. It uses mailbox not texture_id. Besides, we don't need many things there like coded size and visible size. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:51: // The designed size of each window. Sorry, it should be "desired". We may resize the window in order to show all streams on the screen. The window_sizes is also used for the thumbnail mode. So, I rephrase the comment a little bit. On 2014/08/14 04:48:23, Pawel Osciak wrote: > s/designed// > > Please explain this is used for the regular playback mode with each stream > playing in its own window on the screen, and the members below that they are > used for the thumbnail mode where all frames are rendered in sequence onto one > FBO for comparison/verification purposes in thumbnail test. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:70: On 2014/08/14 04:48:23, Pawel Osciak wrote: > Not needed. Done. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:96: void ClearVideoFrames(size_t window_id); On 2014/08/14 04:48:23, Pawel Osciak wrote: > Please document and rename to DropPendingFrames(). Done. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:114: struct RenderingClient { Done. On 2014/08/14 04:48:23, Pawel Osciak wrote: > I think we should not call it a Client, because this is not a Client, just a > struct with data for clients. Otherwise it would imply that we owned the clients > by having a vector of them below. > > How about RenderedVideo ? https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:115: gfx::Rect render_area; On 2014/08/14 04:48:23, Pawel Osciak wrote: > Please documentation for all members. Done. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:116: bool last_frame_rendered; On 2014/08/14 04:48:23, Pawel Osciak wrote: > Isn't this equivalent to pending_frames.empty() ? Why? do you mean pending_frames.size() == 1? We will keep the last frame even it has been rendered. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:118: std::deque<VideoFrame*> pending_frames; On 2014/08/14 04:48:23, Pawel Osciak wrote: > Please keep scoped_refptrs here. Done. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:120: inline RenderingClient() : last_frame_rendered(false) {} On 2014/08/14 04:48:23, Pawel Osciak wrote: > s/inline// Done. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:131: On 2014/08/14 04:48:23, Pawel Osciak wrote: > Remove empty line please. Done. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:441: new media::PictureBuffer(id, dimensions, texture_id); On 2014/08/14 04:48:24, Pawel Osciak wrote: > Could we use linked_ptr here instead of new/delete? Let's do it in another CL. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:467: frame_delivery_times_.push_back(now); On 2014/08/14 04:48:23, Pawel Osciak wrote: > Yay! We will need to change the autotest code to reflect this change. :) https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:1380: CommandLine::Init(argc, argv); On 2014/08/14 04:48:24, Pawel Osciak wrote: > Please don't remove base:: git cl format did it for me. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:1387: CommandLine* cmd_line = CommandLine::ForCurrentProcess(); On 2014/08/14 04:48:24, Pawel Osciak wrote: > nit: const Again, git cl format did it.
https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:172: clients_.resize(params.window_sizes.size()); On 2014/08/14 09:20:41, Owen Lin wrote: > On 2014/08/14 04:48:22, Pawel Osciak wrote: > > This should go to LayoutRenderingAreas() maybe? > mmm... I would prefer keep it here. As the name suggesting, it only do the > layout. We may need other initialization for all the clients. Ok, my issue was that we use videos_.size and window_sizes.size() exchangeably in LayoutRenderingAreas. Would prefer to defensively ensure they are equal somehow. Up to you how. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:28: class VideoFrame { On 2014/08/14 09:20:41, Owen Lin wrote: > On 2014/08/14 04:48:23, Pawel Osciak wrote: > > Why not use media::VideoFrame? > > The main reason is I don't know how to use it in our case. It uses mailbox not > texture_id. > > Besides, we don't need many things there like coded size and visible size. Ok, texture_id is an issue (the other is not really ;)). Let's just please rename it to VideoFrameTexture or something similar so that nobody confuses it with media::VideoFrame. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.h:116: bool last_frame_rendered; On 2014/08/14 09:20:41, Owen Lin wrote: > On 2014/08/14 04:48:23, Pawel Osciak wrote: > > Isn't this equivalent to pending_frames.empty() ? > > Why? do you mean pending_frames.size() == 1? We will keep the last frame even it > has been rendered. Sorry, this was something that I forgot to remove before posting my comments for previous PS. Please ignore. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:441: new media::PictureBuffer(id, dimensions, texture_id); On 2014/08/14 09:20:42, Owen Lin wrote: > On 2014/08/14 04:48:24, Pawel Osciak wrote: > > Could we use linked_ptr here instead of new/delete? > Let's do it in another CL. Acknowledged. https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:467: frame_delivery_times_.push_back(now); On 2014/08/14 09:20:41, Owen Lin wrote: > On 2014/08/14 04:48:23, Pawel Osciak wrote: > > Yay! > > We will need to change the autotest code to reflect this change. :) Still :) https://codereview.chromium.org/462413004/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_decode_accelerator_unittest.cc:1380: CommandLine::Init(argc, argv); On 2014/08/14 09:20:42, Owen Lin wrote: > On 2014/08/14 04:48:24, Pawel Osciak wrote: > > Please don't remove base:: > > git cl format did it for me. We just had crrev.com/288787, so I think we are supposed to keep it. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:394: DCHECK(!video->pending_frames.empty()); Please comment why, this is not obvious. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:396: video->last_frame_rendered = false; By the way, for the future, we should have some thread checking in this class to ensure we have no races. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:499: // To keep the frame before SwapBuffers() This is a bit too vague. Maybe: "Frames that will no longer be needed/will be returned after rendering. We don't return a rendered frame if it's the last pending frame on the queue and we have to retain it in case client falls behind and we need to render the same frame again on the next timer tick." https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:517: frames_to_be_returned.push_back(video->pending_frames.front()); s/video->pending_frames.front()/frame/ https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:520: video->last_frame_rendered = true; Isn't this a problem in QueueVideoFrame? We may return a frame that hasn't been rendered yet if size > 1 and client is queuing faster than we can render? https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.h:122: // Ture if the last (and the only one) frame in pending_frames has s/Ture/True/ Also, I think you want to say not the last, but the oldest/front frame. Even if we have more than one frame in pending, we still use last_frame_rendered to know if we can return it... Also please add: We keep the last remaining frame in pending_frames even after it has been rendered, so that we have something to display if the client is falling behind on providing us with new frames during timer-driven playback. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.h:127: std::deque<scoped_refptr<VideoFrame> > pending_frames; Oh I forgot to comment on this last time, you don't really need a deque I think, std::queue should be enough? https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (left): https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:640: void GLRenderingVDAClient::NotifyFrameDropped(int32 picture_buffer_id) { Oh, this was never used? How do we track dropped frames? https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:502: rendering_helper_->RenderThumbnail(video_frame->texture_target(), I'm a little bit lost in the thumbnail code still, so maybe I'm missing something. Why not use QueueVideoFrame for this as well?
Thanks. PTAL. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:394: DCHECK(!video->pending_frames.empty()); On 2014/08/15 05:45:16, Pawel Osciak wrote: > Please comment why, this is not obvious. Done. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:396: video->last_frame_rendered = false; On 2014/08/15 05:45:16, Pawel Osciak wrote: > By the way, for the future, we should have some thread checking in this class to > ensure we have no races. Acknowledged. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:499: // To keep the frame before SwapBuffers() On 2014/08/15 05:45:16, Pawel Osciak wrote: > This is a bit too vague. Maybe: "Frames that will no longer be needed/will be > returned after rendering. We don't return a rendered frame if it's the last > pending frame on the queue and we have to retain it in case client falls behind > and we need to render the same frame again on the next timer tick." The same comment was added to the member "last_frame_rendered". I think we need "frames_to_be_returned" is to keep the frame so that it won't be reused before SwapBuffer. I have rephrased the sentence to explain more. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:517: frames_to_be_returned.push_back(video->pending_frames.front()); On 2014/08/15 05:45:16, Pawel Osciak wrote: > s/video->pending_frames.front()/frame/ Done. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:520: video->last_frame_rendered = true; On 2014/08/15 05:45:16, Pawel Osciak wrote: > Isn't this a problem in QueueVideoFrame? We may return a frame that hasn't been > rendered yet if size > 1 and client is queuing faster than we can render? No, it won't. We set video_last_frame_rendered = true only when pending_frame.size() == 1 and the frame was just being rendered. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.h:122: // Ture if the last (and the only one) frame in pending_frames has On 2014/08/15 05:45:16, Pawel Osciak wrote: > s/Ture/True/ > > Also, I think you want to say not the last, but the oldest/front frame. Even if > we have more than one frame in pending, we still use last_frame_rendered to know > if we can return it... I have changed the documentation. But I am confused, if we have more than one frame in pending, then last_frame_rendered won't be True. > > Also please add: > > We keep the last remaining frame in pending_frames even after it has been > rendered, so that we have something to display if the client is falling behind > on providing us with new frames during timer-driven playback. Done. Thanks. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.h:127: std::deque<scoped_refptr<VideoFrame> > pending_frames; On 2014/08/15 05:45:16, Pawel Osciak wrote: > Oh I forgot to comment on this last time, you don't really need a deque I think, > std::queue should be enough? Done. Thanks. I got a feeling that deque is more efficient. But maybe that's only for java's implementation. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (left): https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:640: void GLRenderingVDAClient::NotifyFrameDropped(int32 picture_buffer_id) { On 2014/08/15 05:45:16, Pawel Osciak wrote: > Oh, this was never used? How do we track dropped frames? This has to be done in the autotest. https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/462413004/diff/40001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:502: rendering_helper_->RenderThumbnail(video_frame->texture_target(), On 2014/08/15 05:45:16, Pawel Osciak wrote: > I'm a little bit lost in the thumbnail code still, so maybe I'm missing > something. Why not use QueueVideoFrame for this as well? The thumbnail is rendered to the FBO and thus a different path. We don't need to wait for SwapBuffer().
lgtm % nits https://chromiumcodereview.appspot.com/462413004/diff/40001/content/common/gp... File content/common/gpu/media/rendering_helper.cc (right): https://chromiumcodereview.appspot.com/462413004/diff/40001/content/common/gp... content/common/gpu/media/rendering_helper.cc:520: video->last_frame_rendered = true; On 2014/08/18 09:03:08, Owen Lin wrote: > On 2014/08/15 05:45:16, Pawel Osciak wrote: > > Isn't this a problem in QueueVideoFrame? We may return a frame that hasn't > been > > rendered yet if size > 1 and client is queuing faster than we can render? > > No, it won't. We set video_last_frame_rendered = true only when > pending_frame.size() == 1 and the frame was just being rendered. Ok now I know why I misunderstood this. You are using last_frame_rendered to mean both that there is only one frame in the pending_frames queue, as well as the fact that it was rendered. This is confusing that it is used to indicate there is only one frame left in the queue. I would prefer if we could please use pending_frames.count() to detect that we only have one frame left at l.394 instead of the DCHECK. Otherwise it's hard to understand that this happens. I know it's redundant, but I think in this case it helps understanding. https://chromiumcodereview.appspot.com/462413004/diff/40001/content/common/gp... File content/common/gpu/media/rendering_helper.h (right): https://chromiumcodereview.appspot.com/462413004/diff/40001/content/common/gp... content/common/gpu/media/rendering_helper.h:122: // Ture if the last (and the only one) frame in pending_frames has On 2014/08/18 09:03:08, Owen Lin wrote: > On 2014/08/15 05:45:16, Pawel Osciak wrote: > > s/Ture/True/ > > > > Also, I think you want to say not the last, but the oldest/front frame. Even > if > > we have more than one frame in pending, we still use last_frame_rendered to > know > > if we can return it... > > I have changed the documentation. But I am confused, if we have more than one > frame in pending, then last_frame_rendered won't be True. Right, it was actually me who was confused. Please see my comment in the other file about this. https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... File content/common/gpu/media/rendering_helper.cc (right): https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.cc:16: #include "base/stl_util.h" I think this is not needed anymore now that we use scoped refptrs to delete? https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.cc:393: // Pop the front if it has been rendered. This is what is confusing. Because this suggests that last_frame_rendered applies to any front frame, not only if size == 1. https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.cc:395: // When last_frame_rendered is True, we should have only one pending frame. s/True/true/ https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.cc:502: // To keep the frames before calling SwapBuffers(). Otherwise, those frames I think this would be easier to understand: "Frames that will be returned to the client (via the no longer_needed_cb) after this vector falls out of scope at the end of this method. We need to keep refs to them until after SwapBuffers() call below." https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... File content/common/gpu/media/rendering_helper.h (right): https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.h:96: // Queues the video_frame for rendering. Nit: |video_frame| https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.h:123: // True if the oldest (and the only one) frame in pending_frames has Sorry, actually it should be the newest, my bad. But maybe better just say "last". https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.h:124: // been rendered. We keep the last remaining frame in pending_frames even s/even/even after/
https://chromiumcodereview.appspot.com/462413004/diff/40001/content/common/gp... File content/common/gpu/media/rendering_helper.cc (right): https://chromiumcodereview.appspot.com/462413004/diff/40001/content/common/gp... content/common/gpu/media/rendering_helper.cc:520: video->last_frame_rendered = true; We will keep at most one frame for delayed decoding (render the last frame again). So, last_frame_rendered don't have to be true if there are more than one frames (In that case, we will drop the rendered frame and set last_frame_rendered to false). Did I convince you it should be a DCHECK ? :) On 2014/08/20 10:26:26, Pawel Osciak wrote: > On 2014/08/18 09:03:08, Owen Lin wrote: > > On 2014/08/15 05:45:16, Pawel Osciak wrote: > > > Isn't this a problem in QueueVideoFrame? We may return a frame that hasn't > > been > > > rendered yet if size > 1 and client is queuing faster than we can render? > > > > No, it won't. We set video_last_frame_rendered = true only when > > pending_frame.size() == 1 and the frame was just being rendered. > > Ok now I know why I misunderstood this. You are using last_frame_rendered to > mean both that there is only one frame in the pending_frames queue, as well as > the fact that it was rendered. This is confusing that it is used to indicate > there is only one frame left in the queue. I would prefer if we could please use > pending_frames.count() to detect that we only have one frame left at l.394 > instead of the DCHECK. Otherwise it's hard to understand that this happens. I > know it's redundant, but I think in this case it helps understanding. https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... File content/common/gpu/media/rendering_helper.cc (right): https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.cc:16: #include "base/stl_util.h" On 2014/08/20 10:26:26, Pawel Osciak wrote: > I think this is not needed anymore now that we use scoped refptrs to delete? Done. https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.cc:393: // Pop the front if it has been rendered. On 2014/08/20 10:26:26, Pawel Osciak wrote: > This is what is confusing. Because this suggests that last_frame_rendered > applies to any front frame, not only if size == 1. Rephrased. Thanks. https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.cc:395: // When last_frame_rendered is True, we should have only one pending frame. On 2014/08/20 10:26:26, Pawel Osciak wrote: > s/True/true/ Done. https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.cc:502: // To keep the frames before calling SwapBuffers(). Otherwise, those frames On 2014/08/20 10:26:26, Pawel Osciak wrote: > I think this would be easier to understand: > > "Frames that will be returned to the client (via the no longer_needed_cb) after > this vector falls out of scope at the end of this method. We need to keep refs > to them until after SwapBuffers() call below." Thanks. https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... File content/common/gpu/media/rendering_helper.h (right): https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.h:96: // Queues the video_frame for rendering. On 2014/08/20 10:26:26, Pawel Osciak wrote: > Nit: |video_frame| Done. https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.h:123: // True if the oldest (and the only one) frame in pending_frames has On 2014/08/20 10:26:26, Pawel Osciak wrote: > Sorry, actually it should be the newest, my bad. But maybe better just say > "last". Done. https://chromiumcodereview.appspot.com/462413004/diff/60001/content/common/gp... content/common/gpu/media/rendering_helper.h:124: // been rendered. We keep the last remaining frame in pending_frames even On 2014/08/20 10:26:26, Pawel Osciak wrote: > s/even/even after/ Done.
The CQ bit was checked by owenlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/owenlin@chromium.org/462413004/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by owenlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/owenlin@chromium.org/462413004/100001
The CQ bit was checked by owenlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/owenlin@chromium.org/462413004/120001
Message was sent while issue was closed.
Committed patchset #6 (120001) as 291061 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
