Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
271593010:
rendering_helper - Replaces multiple windows by a full-screen window. (Closed)
|
Created:
6 years, 7 months ago by Owen Lin Modified:
6 years, 6 months ago CC:
chromium-reviews, piman+watch_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionrendering_helper - Replace multiple windows by a full-screen window.
On some ChromeOS devices, there would be extra effert to copy content
from a window to the screen when the window is not full-sized.
This change tries to replace the windows by a full-screen window and
let different decoders draw on different areas of the window.
This did improve the performance a lot. After this change, the time
took by the thumbnail test is improved from 7 seconds to 1.6 seconds.
Due to crbug/270064, we cannot run two fullscreen X apps. The chrome
should be stopped before unning the vda_unittest.
# touch /var/run/disable_chrome_restrat
# restart ui
BUG=chrome-os-partner:28176
TEST=Run the test on peach-pit
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276025
Patch Set 1 #Patch Set 2 : #
Total comments: 21
Patch Set 3 : address review comments #Patch Set 4 : fix typos and run git cl format #
Total comments: 8
Patch Set 5 : address Pawel's comment. #Patch Set 6 : fix typos for windows platfrom #Patch Set 7 : fix compiling errors on windows. #Patch Set 8 : fix compiling error. #
Messages
Total messages: 29 (0 generated)
PTAL. This is the first step. Next, I will try to remove ThrottledVDAClient since we have rendering_helper to control the fps now.
Defer the rest of this review to posciak/yuli. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:22: const int kDefaultWindowHeight = 720; Can you drop these in favor of WIN APIs for discovering the screen-size? https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:163: Screen *scrn = DefaultScreenOfDisplay(x_display_); s/scrn/screen/ (ditto elsewhere chromium & google3 style eschews arbitrary shortenings like this) https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:221: texture_targets_.push_back(0); These two lines look strange. Can you doco why they exist? (or better yet rewrite so they don't need to) https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:257: // TODO (owenlin): Do we still this if we have only one context? probably not https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:305: texture_ids_[0] = thumbnails_texture_id_; ditto unclear what these lines are about. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:581: if (!rendering_state_ == RENDERING_STOPPING) { !foo==blah doesn't look right... https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:587: // esure the fps typo: esure https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:587: // esure the fps end sentences with periods https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:606: void RenderingHelper::StartRendering() { This start/stop/PostDelayedTask scheme seems a bit more verbose than it needs to be. I think a base::RepeatingTimer would let you get away with much less code (and clearer stop semantics). https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. CL description has a bunch of typos; please fix https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.h:32: enum RenderingState { only used by the impl; should be in the private: section of RenderingHelper.
https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:22: const int kDefaultWindowHeight = 720; On 2014/05/08 21:15:09, Ami Fischman wrote: > Can you drop these in favor of WIN APIs for discovering the screen-size? Done. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:163: Screen *scrn = DefaultScreenOfDisplay(x_display_); On 2014/05/08 21:15:09, Ami Fischman wrote: > s/scrn/screen/ > (ditto elsewhere chromium & google3 style eschews arbitrary shortenings like > this) Done. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:221: texture_targets_.push_back(0); On 2014/05/08 21:15:09, Ami Fischman wrote: > These two lines look strange. Can you doco why they exist? > (or better yet rewrite so they don't need to) Comments added. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:257: // TODO (owenlin): Do we still this if we have only one context? On 2014/05/08 21:15:09, Ami Fischman wrote: > probably not I just test it, we still need to make it current once. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:305: texture_ids_[0] = thumbnails_texture_id_; On 2014/05/08 21:15:09, Ami Fischman wrote: > ditto unclear what these lines are about. Document added. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:581: if (!rendering_state_ == RENDERING_STOPPING) { On 2014/05/08 21:15:09, Ami Fischman wrote: > !foo==blah doesn't look right... :) Thanks. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:587: // esure the fps On 2014/05/08 21:15:09, Ami Fischman wrote: > typo: esure Done. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:587: // esure the fps On 2014/05/08 21:15:09, Ami Fischman wrote: > end sentences with periods Done. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:606: void RenderingHelper::StartRendering() { On 2014/05/08 21:15:09, Ami Fischman wrote: > This start/stop/PostDelayedTask scheme seems a bit more verbose than it needs to > be. I think a base::RepeatingTimer would let you get away with much less code > (and clearer stop semantics). Thanks. It is a nice tool. https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/271593010/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.h:32: enum RenderingState { On 2014/05/08 21:15:09, Ami Fischman wrote: > only used by the impl; should be in the private: section of RenderingHelper. Done.
https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:189: // Per-window/surface X11 & EGL initialization. CHECK(texture_ids_.empty()); CHECK(texture_targets_.empty()); https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:203: texture_ids_.push_back(0); You could achieve the same by saying outside of this loop: texture_ids_.resize(params.num_windows, 0); (assuming texture_ids.empty(), but you require this anyway). https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:474: texture_ids_[window_id] = texture_id; It looks to me that RenderTexture() used to be synchronous, i.e. once it returned, the texture was fully read out and rendered, since we had SwapBuffers at the end of it. So the vdatest could immediately return the texture to VDA class via ReusePictureBuffer once this method returned. But now it's not the case, so you could have VDA overwrite the contents of this texture before you get to render it out in DrawTexture on next timer tick. https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.h:101: base::RepeatingTimer<RenderingHelper> render_timer_; Document please?
https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:203: texture_ids_.push_back(0); On 2014/05/27 06:21:49, Pawel Osciak wrote: > You could achieve the same by saying outside of this loop: > > texture_ids_.resize(params.num_windows, 0); > > (assuming texture_ids.empty(), but you require this anyway). Thanks. https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:474: texture_ids_[window_id] = texture_id; On 2014/05/27 06:21:49, Pawel Osciak wrote: > It looks to me that RenderTexture() used to be synchronous, i.e. once it > returned, the texture was fully read out and rendered, since we had SwapBuffers > at the end of it. > > So the vdatest could immediately return the texture to VDA class via > ReusePictureBuffer once this method returned. > > But now it's not the case, so you could have VDA overwrite the contents of this > texture before you get to render it out in DrawTexture on next timer tick. Ah, you are right. There is problem in this code. However, since this issue will be addressed in the next CL. https://codereview.chromium.org/294663006/ Are you OK that I add a TODO item here. Or I can just merge these two CLs before submitting ? https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.h (right): https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.h:101: base::RepeatingTimer<RenderingHelper> render_timer_; On 2014/05/27 06:21:49, Pawel Osciak wrote: > Document please? Done.
https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:474: texture_ids_[window_id] = texture_id; On 2014/05/27 07:33:51, Owen Lin wrote: > On 2014/05/27 06:21:49, Pawel Osciak wrote: > > It looks to me that RenderTexture() used to be synchronous, i.e. once it > > returned, the texture was fully read out and rendered, since we had > SwapBuffers > > at the end of it. > > > > So the vdatest could immediately return the texture to VDA class via > > ReusePictureBuffer once this method returned. > > > > But now it's not the case, so you could have VDA overwrite the contents of > this > > texture before you get to render it out in DrawTexture on next timer tick. > > Ah, you are right. There is problem in this code. However, since this issue will > be addressed in the next CL. > https://codereview.chromium.org/294663006/ > > Are you OK that I add a TODO item here. Or I can just merge these two CLs before > submitting ? Hm, I guess merging is probably better once we lgtm both, unless Ami has a different preference.
On 2014/05/28 05:45:51, Pawel Osciak wrote: > https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... > File content/common/gpu/media/rendering_helper.cc (right): > > https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... > content/common/gpu/media/rendering_helper.cc:474: texture_ids_[window_id] = > texture_id; > On 2014/05/27 07:33:51, Owen Lin wrote: > > On 2014/05/27 06:21:49, Pawel Osciak wrote: > > > It looks to me that RenderTexture() used to be synchronous, i.e. once it > > > returned, the texture was fully read out and rendered, since we had > > SwapBuffers > > > at the end of it. > > > > > > So the vdatest could immediately return the texture to VDA class via > > > ReusePictureBuffer once this method returned. > > > > > > But now it's not the case, so you could have VDA overwrite the contents of > > this > > > texture before you get to render it out in DrawTexture on next timer tick. > > > > Ah, you are right. There is problem in this code. However, since this issue > will > > be addressed in the next CL. > > https://codereview.chromium.org/294663006/ > > > > Are you OK that I add a TODO item here. Or I can just merge these two CLs > before > > submitting ? > > Hm, I guess merging is probably better once we lgtm both, unless Ami has a > different preference. I have a temporal fix for it. Please take a look. Thanks.
On 2014/05/28 08:01:20, Owen Lin wrote: > On 2014/05/28 05:45:51, Pawel Osciak wrote: > > > https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... > > File content/common/gpu/media/rendering_helper.cc (right): > > > > > https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... > > content/common/gpu/media/rendering_helper.cc:474: texture_ids_[window_id] = > > texture_id; > > On 2014/05/27 07:33:51, Owen Lin wrote: > > > On 2014/05/27 06:21:49, Pawel Osciak wrote: > > > > It looks to me that RenderTexture() used to be synchronous, i.e. once it > > > > returned, the texture was fully read out and rendered, since we had > > > SwapBuffers > > > > at the end of it. > > > > > > > > So the vdatest could immediately return the texture to VDA class via > > > > ReusePictureBuffer once this method returned. > > > > > > > > But now it's not the case, so you could have VDA overwrite the contents of > > > this > > > > texture before you get to render it out in DrawTexture on next timer tick. > > > > > > Ah, you are right. There is problem in this code. However, since this issue > > will > > > be addressed in the next CL. > > > https://codereview.chromium.org/294663006/ > > > > > > Are you OK that I add a TODO item here. Or I can just merge these two CLs > > before > > > submitting ? > > > > Hm, I guess merging is probably better once we lgtm both, unless Ami has a > > different preference. > > I have a temporal fix for it. Please take a look. Thanks. I don't think this fixe it. It delays ReusePictureBuffer, but if PictureReady is called again while the texture is still being rendered, the same problem occurs, it's just delayed.
On 2014/05/29 08:18:14, Pawel Osciak wrote: > On 2014/05/28 08:01:20, Owen Lin wrote: > > On 2014/05/28 05:45:51, Pawel Osciak wrote: > > > > > > https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... > > > File content/common/gpu/media/rendering_helper.cc (right): > > > > > > > > > https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... > > > content/common/gpu/media/rendering_helper.cc:474: texture_ids_[window_id] = > > > texture_id; > > > On 2014/05/27 07:33:51, Owen Lin wrote: > > > > On 2014/05/27 06:21:49, Pawel Osciak wrote: > > > > > It looks to me that RenderTexture() used to be synchronous, i.e. once it > > > > > returned, the texture was fully read out and rendered, since we had > > > > SwapBuffers > > > > > at the end of it. > > > > > > > > > > So the vdatest could immediately return the texture to VDA class via > > > > > ReusePictureBuffer once this method returned. > > > > > > > > > > But now it's not the case, so you could have VDA overwrite the contents > of > > > > this > > > > > texture before you get to render it out in DrawTexture on next timer > tick. > > > > > > > > Ah, you are right. There is problem in this code. However, since this > issue > > > will > > > > be addressed in the next CL. > > > > https://codereview.chromium.org/294663006/ > > > > > > > > Are you OK that I add a TODO item here. Or I can just merge these two CLs > > > before > > > > submitting ? > > > > > > Hm, I guess merging is probably better once we lgtm both, unless Ami has a > > > different preference. > > > > I have a temporal fix for it. Please take a look. Thanks. > > I don't think this fixe it. It delays ReusePictureBuffer, but if PictureReady is > called again while the texture is still being rendered, the same problem occurs, > it's just delayed. According to my understanding, both PictureReady and RenderTexture are executed in the rendering thread. So, this should not be a problem.
On 2014/05/30 01:13:46, Owen Lin wrote: > On 2014/05/29 08:18:14, Pawel Osciak wrote: > > On 2014/05/28 08:01:20, Owen Lin wrote: > > > On 2014/05/28 05:45:51, Pawel Osciak wrote: > > > > > > > > > > https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... > > > > File content/common/gpu/media/rendering_helper.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... > > > > content/common/gpu/media/rendering_helper.cc:474: texture_ids_[window_id] > = > > > > texture_id; > > > > On 2014/05/27 07:33:51, Owen Lin wrote: > > > > > On 2014/05/27 06:21:49, Pawel Osciak wrote: > > > > > > It looks to me that RenderTexture() used to be synchronous, i.e. once > it > > > > > > returned, the texture was fully read out and rendered, since we had > > > > > SwapBuffers > > > > > > at the end of it. > > > > > > > > > > > > So the vdatest could immediately return the texture to VDA class via > > > > > > ReusePictureBuffer once this method returned. > > > > > > > > > > > > But now it's not the case, so you could have VDA overwrite the > contents > > of > > > > > this > > > > > > texture before you get to render it out in DrawTexture on next timer > > tick. > > > > > > > > > > Ah, you are right. There is problem in this code. However, since this > > issue > > > > will > > > > > be addressed in the next CL. > > > > > https://codereview.chromium.org/294663006/ > > > > > > > > > > Are you OK that I add a TODO item here. Or I can just merge these two > CLs > > > > before > > > > > submitting ? > > > > > > > > Hm, I guess merging is probably better once we lgtm both, unless Ami has a > > > > different preference. > > > > > > I have a temporal fix for it. Please take a look. Thanks. > > > > I don't think this fixe it. It delays ReusePictureBuffer, but if PictureReady > is > > called again while the texture is still being rendered, the same problem > occurs, > > it's just delayed. > > According to my understanding, both PictureReady and RenderTexture are executed > in the rendering thread. So, this should not be a problem. RenderTexture submits the job to the GPU, but the GPU renders asynchronously from anything that happens in Chrome. The SwapBuffers call that we had previously in RenderTexture was what made us synchronized with the GPU.
On 2014/05/30 01:25:57, Pawel Osciak wrote: > On 2014/05/30 01:13:46, Owen Lin wrote: > > On 2014/05/29 08:18:14, Pawel Osciak wrote: > > > On 2014/05/28 08:01:20, Owen Lin wrote: > > > > On 2014/05/28 05:45:51, Pawel Osciak wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... > > > > > File content/common/gpu/media/rendering_helper.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/271593010/diff/130001/content/common/gpu/medi... > > > > > content/common/gpu/media/rendering_helper.cc:474: > texture_ids_[window_id] > > = > > > > > texture_id; > > > > > On 2014/05/27 07:33:51, Owen Lin wrote: > > > > > > On 2014/05/27 06:21:49, Pawel Osciak wrote: > > > > > > > It looks to me that RenderTexture() used to be synchronous, i.e. > once > > it > > > > > > > returned, the texture was fully read out and rendered, since we had > > > > > > SwapBuffers > > > > > > > at the end of it. > > > > > > > > > > > > > > So the vdatest could immediately return the texture to VDA class via > > > > > > > ReusePictureBuffer once this method returned. > > > > > > > > > > > > > > But now it's not the case, so you could have VDA overwrite the > > contents > > > of > > > > > > this > > > > > > > texture before you get to render it out in DrawTexture on next timer > > > tick. > > > > > > > > > > > > Ah, you are right. There is problem in this code. However, since this > > > issue > > > > > will > > > > > > be addressed in the next CL. > > > > > > https://codereview.chromium.org/294663006/ > > > > > > > > > > > > Are you OK that I add a TODO item here. Or I can just merge these two > > CLs > > > > > before > > > > > > submitting ? > > > > > > > > > > Hm, I guess merging is probably better once we lgtm both, unless Ami has > a > > > > > different preference. > > > > > > > > I have a temporal fix for it. Please take a look. Thanks. > > > > > > I don't think this fixe it. It delays ReusePictureBuffer, but if > PictureReady > > is > > > called again while the texture is still being rendered, the same problem > > occurs, > > > it's just delayed. > > > > According to my understanding, both PictureReady and RenderTexture are > executed > > in the rendering thread. So, this should not be a problem. > > RenderTexture submits the job to the GPU, but the GPU renders asynchronously > from anything that happens in Chrome. > The SwapBuffers call that we had previously in RenderTexture was what made us > synchronized with the GPU. Thanks. I see. I will merge this CL into next before submitting.
Hi, Scherkus, Can you help us review the CL? Pawel and I has discussed about the temporary solution offline and we think it should work.
rs lgtm
The CQ bit was checked by owenlin@chromium.org
The CQ bit was unchecked by owenlin@chromium.org
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/271593010/150001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by owenlin@chromium.org
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/271593010/170001
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/271593010/190001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
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/271593010/210001
Message was sent while issue was closed.
Change committed as 276025 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
