Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2150833002:
[Chromoting] Fallback to use software renderer if desktop image size is larger than GPU limitaiton (Closed)
|
Created:
4 years, 5 months ago by Hzj_jie Modified:
4 years, 2 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, posciak+watch_chromium.org, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromoting] Fallback to use software renderer if desktop image size is larger
than GPU limitation
On some old or lowend hardwares, such as Pixel 1 (ChromeBook), the max texture
their GPUs can process may smaller than a combination of several high resolution
monitors. In this situation, GPU decoder will fail, and we will render a blank
white screen. So this change adds a check of max texture size GPU (from OpenGL
layer) supports. And if a desktop image is larger than this limitation,
PepperVideoRenderer3D will fall back to PepperVideoRenderer2D to render
following images.
BUG=584388
Committed: https://crrev.com/103f52ed2f2d52af0554f6817ebef53e1b1eff4a
Cr-Commit-Position: refs/heads/master@{#426019}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Resolve review comments #Patch Set 3 : Resolve review comments #
Total comments: 3
Patch Set 4 : Resolve review comments #Patch Set 5 : Use scaled gl limitation #
Total comments: 12
Patch Set 6 : Resolve review comments #Patch Set 7 : Resolve review comments #
Total comments: 11
Patch Set 8 : Resolve review comments #
Messages
Total messages: 77 (57 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Chromoting] Fallback to use software renderer if desktop image size is larger than GPU max texture limitation On some old or lowend hardwares, such as Pixel 1 (ChromeBook), the max texture their GPUs can process may smaller than a combination of several high resolution monitors. In this situation, GPU decoder will fail, and we will render a blank white screen. So this change adds a check of max texture size GPU (from OpenGL layer) supports. And if a desktop image is larger than this limitation, PepperVideoRenderer3D will fall back to PepperVideoRenderer2D to render following images. A specific note for DesktopSessionAgent change. Since the mouse cursor is also rendered by Pepper, a switching between 3D and 2D renderers will clear existing cursor shape. And the client won't get a cursor until next cursor shape change. So the change to DesktopSessionAgent makes sure the first desktop image will be sent before the cursor shape. And client can decide the right renderer to use before render the cursor shape. BUG=584388 ========== to ========== [Chromoting] Fallback to use software renderer if desktop image size is larger than GPU limitation On some old or lowend hardwares, such as Pixel 1 (ChromeBook), the max texture their GPUs can process may smaller than a combination of several high resolution monitors. In this situation, GPU decoder will fail, and we will render a blank white screen. So this change adds a check of max texture size GPU (from OpenGL layer) supports. And if a desktop image is larger than this limitation, PepperVideoRenderer3D will fall back to PepperVideoRenderer2D to render following images. A specific note for DesktopSessionAgent change. Since the mouse cursor is also rendered by Pepper, a switching between 3D and 2D renderers will clear existing cursor shape. And the client won't get a cursor until next cursor shape change. So the change to DesktopSessionAgent makes sure the first desktop image will be sent before the cursor shape. And client can decide the right renderer to use before render the cursor shape. BUG=584388 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.cc:227: use_fallback_renderer_ = Once we start using the fallback renderer I think we want to keep using it. Switching back and forth between them when resolution changes may not be safe. Also I think we need to free all old pending frames and all resources used by the 3D renderer before the fallback happens. Freeing all FrameTracker objects is especially important as they will call Done callback. current_picture_frames_.clear(); current_picture_.reset() next_picture_frames_.clear(); next_picture_.reset(); decoded_frames_.clear(); pending_frames_.clear(); // Reset Pepper resources, freeing the old ones. graphics_ = pp::Graphics3D(); video_renderer_ = pp::VideoDecoder(); https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.cc:228: gl_max_texture_size_ < packet->format().screen_width() || nit: would be more readable if this condition was in the opposite order: packet->format().screen_width() > gl_max_texture_size_ https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.cc:228: gl_max_texture_size_ < packet->format().screen_width() || screen_width() and screen_height() are set only when size changes. Code below uses .has_screen_width() https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.h:16: #include "ppapi/c/ppb_opengles2.h" Don't need this. Use int instead of GLint below. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.h:143: PepperVideoRenderer2D fallback_renderer_; I don't think we want to allocate fallback renderer until we actually need it. Make this a pointer. Then you wouldn't need use_fallback_renderer_. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.h:144: bool fallback_renderer_usable_; The 2D renderer is not expected to fail. Just add a CHECK() to verify it's initialized correctly. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.h:167: GLint gl_max_texture_size_; int https://codereview.chromium.org/2150833002/diff/1/remoting/host/desktop_sessi... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/2150833002/diff/1/remoting/host/desktop_sessi... remoting/host/desktop_session_agent.cc:470: mouse_cursor_monitor_->Capture(); This doesn't guarantee that the cursor will be received after the first frame. Cursor shape and video are sent on different channels, independent of each other, so capture ordering here will have very little effect on how the messages are received on the client. But I don't see how it's related to this change. VideoRenderer is not responsible for the cursor.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.cc:227: use_fallback_renderer_ = On 2016/07/14 17:45:28, Sergey Ulanov wrote: > Once we start using the fallback renderer I think we want to keep using it. > Switching back and forth between them when resolution changes may not be safe. > > Also I think we need to free all old pending frames and all resources used by > the 3D renderer before the fallback happens. Freeing all FrameTracker objects is > especially important as they will call Done callback. > > current_picture_frames_.clear(); > current_picture_.reset() > next_picture_frames_.clear(); > next_picture_.reset(); > decoded_frames_.clear(); > pending_frames_.clear(); > > // Reset Pepper resources, freeing the old ones. > graphics_ = pp::Graphics3D(); > video_renderer_ = pp::VideoDecoder(); > > No, if use_fallback_renderer_ is true, it won't fallback. Others are added. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.cc:228: gl_max_texture_size_ < packet->format().screen_width() || On 2016/07/14 17:45:28, Sergey Ulanov wrote: > screen_width() and screen_height() are set only when size changes. Code below > uses .has_screen_width() I believe in protobuf, an unset integer field is always zero. And gl_max_texture_size_ won't be less than zero. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.cc:228: gl_max_texture_size_ < packet->format().screen_width() || On 2016/07/14 17:45:28, Sergey Ulanov wrote: > nit: would be more readable if this condition was in the opposite order: > packet->format().screen_width() > gl_max_texture_size_ Done. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.h:16: #include "ppapi/c/ppb_opengles2.h" On 2016/07/14 17:45:28, Sergey Ulanov wrote: > Don't need this. Use int instead of GLint below. Done. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.h:143: PepperVideoRenderer2D fallback_renderer_; On 2016/07/14 17:45:28, Sergey Ulanov wrote: > I don't think we want to allocate fallback renderer until we actually need it. > Make this a pointer. Then you wouldn't need use_fallback_renderer_. I believe SetPepperContext / OnViewChanged / EnableDebugDirtyRegion / Initialize / OnSessionConfig are called before processing the first frame. So we always need to initialize fallback_renderer_ even we do not use it. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.h:144: bool fallback_renderer_usable_; On 2016/07/14 17:45:28, Sergey Ulanov wrote: > The 2D renderer is not expected to fail. Just add a CHECK() to verify it's > initialized correctly. Done. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.h:167: GLint gl_max_texture_size_; On 2016/07/14 17:45:28, Sergey Ulanov wrote: > int Done. https://codereview.chromium.org/2150833002/diff/1/remoting/host/desktop_sessi... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/2150833002/diff/1/remoting/host/desktop_sessi... remoting/host/desktop_session_agent.cc:470: mouse_cursor_monitor_->Capture(); On 2016/07/14 17:45:28, Sergey Ulanov wrote: > This doesn't guarantee that the cursor will be received after the first frame. > Cursor shape and video are sent on different channels, independent of each > other, so capture ordering here will have very little effect on how the messages > are received on the client. > > But I don't see how it's related to this change. VideoRenderer is not > responsible for the cursor. Sorry, my fault. I have double confirmed, the mouse cursor should be able to render. There must be some other issues before. https://codereview.chromium.org/2150833002/diff/40001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/40001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:231: packet->format().screen_height() > gl_max_texture_size_; AFAICT, this check won't work for debug build. gles2_if_->GetIntegerv(GL_MAX_TEXTURE_SIZE) returns a different value for debug build. No idea whether this is an expected behavior.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.cc:228: gl_max_texture_size_ < packet->format().screen_width() || On 2016/07/15 02:30:50, Hzj_jie wrote: > On 2016/07/14 17:45:28, Sergey Ulanov wrote: > > screen_width() and screen_height() are set only when size changes. Code below > > uses .has_screen_width() > > I believe in protobuf, an unset integer field is always zero. And > gl_max_texture_size_ won't be less than zero. It's true, but in general it's usually better to check pretense of the fields explicitly, if only to make it clear in the code that this case was considered. In this particular case it makes difference when only one of the two fields is set: that's incorrect packet and we don't want to initiate fallback in that scenario. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.h:143: PepperVideoRenderer2D fallback_renderer_; On 2016/07/15 02:30:50, Hzj_jie wrote: > On 2016/07/14 17:45:28, Sergey Ulanov wrote: > > I don't think we want to allocate fallback renderer until we actually need it. > > Make this a pointer. Then you wouldn't need use_fallback_renderer_. > > I believe SetPepperContext / OnViewChanged / EnableDebugDirtyRegion / Initialize > / OnSessionConfig are called before processing the first frame. So we always > need to initialize fallback_renderer_ even we do not use it. You do need to call these methods, but they can be called only when fallback_renderer_ is actually needed.
Also please update the description - this CL no longer touches DesktopSessionAgent
https://codereview.chromium.org/2150833002/diff/40001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/40001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:231: packet->format().screen_height() > gl_max_texture_size_; On 2016/07/15 02:30:50, Hzj_jie wrote: > AFAICT, this check won't work for debug build. > gles2_if_->GetIntegerv(GL_MAX_TEXTURE_SIZE) returns a different value for debug > build. No idea whether this is an expected behavior. This is concerning. Is it in debug builds of the webapp or debug builds of chrome? What does glGetIntegerv(GL_MAX_TEXTURE_SIZE) return in debug builds? (you can use LOG(ERROR) in the plugin - it goes to JS console)
Description was changed from ========== [Chromoting] Fallback to use software renderer if desktop image size is larger than GPU limitation On some old or lowend hardwares, such as Pixel 1 (ChromeBook), the max texture their GPUs can process may smaller than a combination of several high resolution monitors. In this situation, GPU decoder will fail, and we will render a blank white screen. So this change adds a check of max texture size GPU (from OpenGL layer) supports. And if a desktop image is larger than this limitation, PepperVideoRenderer3D will fall back to PepperVideoRenderer2D to render following images. A specific note for DesktopSessionAgent change. Since the mouse cursor is also rendered by Pepper, a switching between 3D and 2D renderers will clear existing cursor shape. And the client won't get a cursor until next cursor shape change. So the change to DesktopSessionAgent makes sure the first desktop image will be sent before the cursor shape. And client can decide the right renderer to use before render the cursor shape. BUG=584388 ========== to ========== [Chromoting] Fallback to use software renderer if desktop image size is larger than GPU limitation On some old or lowend hardwares, such as Pixel 1 (ChromeBook), the max texture their GPUs can process may smaller than a combination of several high resolution monitors. In this situation, GPU decoder will fail, and we will render a blank white screen. So this change adds a check of max texture size GPU (from OpenGL layer) supports. And if a desktop image is larger than this limitation, PepperVideoRenderer3D will fall back to PepperVideoRenderer2D to render following images. BUG=584388 ==========
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.cc:228: gl_max_texture_size_ < packet->format().screen_width() || On 2016/07/15 23:33:18, Sergey Ulanov wrote: > On 2016/07/15 02:30:50, Hzj_jie wrote: > > On 2016/07/14 17:45:28, Sergey Ulanov wrote: > > > screen_width() and screen_height() are set only when size changes. Code > below > > > uses .has_screen_width() > > > > I believe in protobuf, an unset integer field is always zero. And > > gl_max_texture_size_ won't be less than zero. > > It's true, but in general it's usually better to check pretense of the fields > explicitly, if only to make it clear in the code that this case was considered. > In this particular case it makes difference when only one of the two fields is > set: that's incorrect packet and we don't want to initiate fallback in that > scenario. Done. https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/2150833002/diff/1/remoting/client/plugin/pepp... remoting/client/plugin/pepper_video_renderer_3d.h:143: PepperVideoRenderer2D fallback_renderer_; On 2016/07/15 23:33:18, Sergey Ulanov wrote: > On 2016/07/15 02:30:50, Hzj_jie wrote: > > On 2016/07/14 17:45:28, Sergey Ulanov wrote: > > > I don't think we want to allocate fallback renderer until we actually need > it. > > > Make this a pointer. Then you wouldn't need use_fallback_renderer_. > > > > I believe SetPepperContext / OnViewChanged / EnableDebugDirtyRegion / > Initialize > > / OnSessionConfig are called before processing the first frame. So we always > > need to initialize fallback_renderer_ even we do not use it. > > You do need to call these methods, but they can be called only when > fallback_renderer_ is actually needed. Oh, that seems unachievable. We do not store most of the input parameters, and some of them are even const reference. We even cannot guarantee whether they are available later. https://codereview.chromium.org/2150833002/diff/40001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/40001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:231: packet->format().screen_height() > gl_max_texture_size_; On 2016/07/15 23:39:56, Sergey Ulanov wrote: > On 2016/07/15 02:30:50, Hzj_jie wrote: > > AFAICT, this check won't work for debug build. > > gles2_if_->GetIntegerv(GL_MAX_TEXTURE_SIZE) returns a different value for > debug > > build. No idea whether this is an expected behavior. > > This is concerning. Is it in debug builds of the webapp or debug builds of > chrome? > What does glGetIntegerv(GL_MAX_TEXTURE_SIZE) return in debug builds? > (you can use LOG(ERROR) in the plugin - it goes to JS console) Sorry, I have made a wrong assumption. Actually we cannot get correct VideoPacketFormat in debug build, the screen_width() and screen_height() are always zero or unset. Since this is a separated issue, I will prepare another change to fix.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zijiehe@chromium.org changed reviewers: - sergeyu@chromium.org
As discussed in the mail thread, we have one more issue which needs to be addressed.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #5 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #5 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Updated, this should be the correct solution.
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
Restart the code review.
https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:105: if (scale == 0) { scale is never expected to be 0. maybe replace it with DCHECK()? https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:109: gl_scaled_limits_.texture_size = gl_limits_.texture_size / scale; I'm not sure this is correct. textures should not be scaled until they are rendered, so I don't see how the scale factor can affect max texture size. If you believe this is correct, maybe add a comment to explain it. https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:127: CHECK(fallback_renderer_.Initialize(context, stats_consumer)); Please change this to if (!fallback_renderer_.Initialize(context, stats_consumer)) LOG(FATAL) << "Failed to initialize fallback_renderer_"; CHECKS() and DCHECKS() should not have side-effects. This code will break if one decides to replace this CHECK with a DCHECK. https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:240: (packet->format().screen_width() > gl_scaled_limits_.texture_size || We only want to initialize fallback when resolution changes - that guarantees that the fallback software decoder will get first key frame after resolution change. https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:242: packet->format().screen_width() > gl_scaled_limits_.viewport_size[0] || It seems strange to compare host screen size with client viewport size. It looks to me that the right thing to do is to compare view_size_ and gl_limits_.viewport_size in OnViewChanged() when the view is resized. Here when the screen gets resized we only need to make sure that the screen fits into a texture. https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:244: if (use_fallback_renderer_) { It would be easier to understand this code if it was written with a single if statement: if (!use_fallback_renderer_ && packet->format().has_screen_width() && .....) { use_fallback_renderer_ = true; ... state cleanup ... }
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #6 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:105: if (scale == 0) { On 2016/10/12 21:53:31, Sergey Ulanov wrote: > scale is never expected to be 0. maybe replace it with DCHECK()? Done. https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:109: gl_scaled_limits_.texture_size = gl_limits_.texture_size / scale; On 2016/10/12 21:53:31, Sergey Ulanov wrote: > I'm not sure this is correct. textures should not be scaled until they are > rendered, so I don't see how the scale factor can affect max texture size. If > you believe this is correct, maybe add a comment to explain it. Yes, the GetDeviceScale returns a scale factor to represent pixels per logical pixel. But hardware knows only hardware pixel. So if an hardware supports only 8192 x 8192, and we want to render a 5000 x 5000 in logic pixel, with scale factor as 2, the hardware will still need to render 10000 x 10000 hardware pixels, which exceeds the limitation. I will add comment to explain this behavior. But these lines have no relationship with the following view_size_.set() statement. I put them here simply because this is the only place we can get a pp::View object. https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:127: CHECK(fallback_renderer_.Initialize(context, stats_consumer)); On 2016/10/12 21:53:31, Sergey Ulanov wrote: > Please change this to > if (!fallback_renderer_.Initialize(context, stats_consumer)) > LOG(FATAL) << "Failed to initialize fallback_renderer_"; > > CHECKS() and DCHECKS() should not have side-effects. This code will break if one > decides to replace this CHECK with a DCHECK. Done. https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:240: (packet->format().screen_width() > gl_scaled_limits_.texture_size || On 2016/10/12 21:53:31, Sergey Ulanov wrote: > We only want to initialize fallback when resolution changes - that guarantees > that the fallback software decoder will get first key frame after resolution > change. Sorry, I cannot quite get the point of your comment. If there is not screen resolution change, the result of this condition won't be impacted. https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:242: packet->format().screen_width() > gl_scaled_limits_.viewport_size[0] || On 2016/10/12 21:53:31, Sergey Ulanov wrote: > It seems strange to compare host screen size with client viewport size. It looks > to me that the right thing to do is to compare view_size_ and > gl_limits_.viewport_size in OnViewChanged() when the view is resized. > Here when the screen gets resized we only need to make sure that the screen fits > into a texture. Have I made a misunderstanding here? I believe the hardware limitation impacts the maximum size of image we are rendering, i.e. packet->format().screen_width() & packet->format().screen_height(), instead of view_size_. And viewport size here is just a name in OpenGl, means the maximum image it can render. https://codereview.chromium.org/2150833002/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:244: if (use_fallback_renderer_) { On 2016/10/12 21:53:31, Sergey Ulanov wrote: > It would be easier to understand this code if it was written with a single if > statement: > if (!use_fallback_renderer_ && > packet->format().has_screen_width() && > .....) { > use_fallback_renderer_ = true; > ... state cleanup ... > } Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:239: packet->format().screen_height() > gl_limits_.texture_size)) { As we have talked offline, I do not think this fallback is really useful, unless we use software decoder in fallback renderer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:105: DCHECK(scale > 0); nit: DCHECK_GT(scale, 0.0); https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:106: view_size_.set(std::min<float>(ceilf(size.width() * scale), I think it's better to use std::min<int> here and cast ceilf() result to int. https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:239: packet->format().screen_height() > gl_limits_.texture_size)) { On 2016/10/17 18:49:57, Hzj_jie wrote: > As we have talked offline, I do not think this fallback is really useful, unless > we use software decoder in fallback renderer. I'm not sure what you mean here. Is your point that viewport size limit is more likely to be exceeded than the texture size limit? https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.h:66: struct GlLimits { I don't think you really need this struct given that it's used in only one place.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:105: DCHECK(scale > 0); On 2016/10/17 20:02:44, Sergey Ulanov wrote: > nit: DCHECK_GT(scale, 0.0); Done. https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:106: view_size_.set(std::min<float>(ceilf(size.width() * scale), On 2016/10/17 20:02:44, Sergey Ulanov wrote: > I think it's better to use std::min<int> here and cast ceilf() result to int. Done. https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:239: packet->format().screen_height() > gl_limits_.texture_size)) { On 2016/10/17 20:02:44, Sergey Ulanov wrote: > On 2016/10/17 18:49:57, Hzj_jie wrote: > > As we have talked offline, I do not think this fallback is really useful, > unless > > we use software decoder in fallback renderer. > > I'm not sure what you mean here. Is your point that viewport size limit is more > likely to be exceeded than the texture size limit? Sorry, I mean pure software renderer as what we have done on Android before OpenGL renderer. Pixel Chromebook still cannot render a frame larger than 8192 pixels with PepperVideoRenderer2D. https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.h:66: struct GlLimits { On 2016/10/17 20:02:44, Sergey Ulanov wrote: > I don't think you really need this struct given that it's used in only one > place. Done.
lgtm https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/2150833002/diff/200001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:239: packet->format().screen_height() > gl_limits_.texture_size)) { On 2016/10/17 22:55:43, Hzj_jie wrote: > On 2016/10/17 20:02:44, Sergey Ulanov wrote: > > On 2016/10/17 18:49:57, Hzj_jie wrote: > > > As we have talked offline, I do not think this fallback is really useful, > > unless > > > we use software decoder in fallback renderer. > > > > I'm not sure what you mean here. Is your point that viewport size limit is > more > > likely to be exceeded than the texture size limit? > > Sorry, I mean pure software renderer as what we have done on Android before > OpenGL renderer. Pixel Chromebook still cannot render a frame larger than 8192 > pixels with PepperVideoRenderer2D. I see. Is it the same on on other client platforms, particularly windows? In either case think this fallback may still useful in some cases, e.g. when Graphics2D is not accelerated.
https://chromiumcodereview.appspot.com/2150833002/diff/200001/remoting/client... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://chromiumcodereview.appspot.com/2150833002/diff/200001/remoting/client... remoting/client/plugin/pepper_video_renderer_3d.cc:239: packet->format().screen_height() > gl_limits_.texture_size)) { On 2016/10/18 03:11:44, Sergey Ulanov wrote: > On 2016/10/17 22:55:43, Hzj_jie wrote: > > On 2016/10/17 20:02:44, Sergey Ulanov wrote: > > > On 2016/10/17 18:49:57, Hzj_jie wrote: > > > > As we have talked offline, I do not think this fallback is really useful, > > > unless > > > > we use software decoder in fallback renderer. > > > > > > I'm not sure what you mean here. Is your point that viewport size limit is > > more > > > likely to be exceeded than the texture size limit? > > > > Sorry, I mean pure software renderer as what we have done on Android before > > OpenGL renderer. Pixel Chromebook still cannot render a frame larger than 8192 > > pixels with PepperVideoRenderer2D. > > I see. Is it the same on on other client platforms, particularly windows? In > either case think this fallback may still useful in some cases, e.g. when > Graphics2D is not accelerated. Windows machines usually have a larger hardware limitation (16312 x 16312), so this issue is not really easy to reproduce on windows client. i.e. we cannot even encode textures over 16312 pixels. But on ChromeOS client, we can easily reproduce it by using a screen resolution larger than 8192 but less than 16312. In this case, both 2D and 3D are not renderable. I also agree to keep this logic to ensure any upgrade to PepperVideoRenderer2D can help to resolve this issue permanently.
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Fallback to use software renderer if desktop image size is larger than GPU limitation On some old or lowend hardwares, such as Pixel 1 (ChromeBook), the max texture their GPUs can process may smaller than a combination of several high resolution monitors. In this situation, GPU decoder will fail, and we will render a blank white screen. So this change adds a check of max texture size GPU (from OpenGL layer) supports. And if a desktop image is larger than this limitation, PepperVideoRenderer3D will fall back to PepperVideoRenderer2D to render following images. BUG=584388 ========== to ========== [Chromoting] Fallback to use software renderer if desktop image size is larger than GPU limitation On some old or lowend hardwares, such as Pixel 1 (ChromeBook), the max texture their GPUs can process may smaller than a combination of several high resolution monitors. In this situation, GPU decoder will fail, and we will render a blank white screen. So this change adds a check of max texture size GPU (from OpenGL layer) supports. And if a desktop image is larger than this limitation, PepperVideoRenderer3D will fall back to PepperVideoRenderer2D to render following images. BUG=584388 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Fallback to use software renderer if desktop image size is larger than GPU limitation On some old or lowend hardwares, such as Pixel 1 (ChromeBook), the max texture their GPUs can process may smaller than a combination of several high resolution monitors. In this situation, GPU decoder will fail, and we will render a blank white screen. So this change adds a check of max texture size GPU (from OpenGL layer) supports. And if a desktop image is larger than this limitation, PepperVideoRenderer3D will fall back to PepperVideoRenderer2D to render following images. BUG=584388 ========== to ========== [Chromoting] Fallback to use software renderer if desktop image size is larger than GPU limitation On some old or lowend hardwares, such as Pixel 1 (ChromeBook), the max texture their GPUs can process may smaller than a combination of several high resolution monitors. In this situation, GPU decoder will fail, and we will render a blank white screen. So this change adds a check of max texture size GPU (from OpenGL layer) supports. And if a desktop image is larger than this limitation, PepperVideoRenderer3D will fall back to PepperVideoRenderer2D to render following images. BUG=584388 Committed: https://crrev.com/103f52ed2f2d52af0554f6817ebef53e1b1eff4a Cr-Commit-Position: refs/heads/master@{#426019} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/103f52ed2f2d52af0554f6817ebef53e1b1eff4a Cr-Commit-Position: refs/heads/master@{#426019} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
