Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
938873002:
Add a new API to create a surfaceless GLSurface for Ozone (Closed)
|
Created:
5 years, 10 months ago by achaulk Modified:
5 years, 9 months ago CC:
chromium-reviews, ozone-reviews_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a new API to create a surfaceless GLSurface for Ozone
Reverting CreateViewGLSurface to creating a surface that works as expected
Chrome uses the new surfaceless API, all others use the old one
BUG=447798
Committed: https://crrev.com/e86e411c3038fd5be67438da29f0ff17828f226f
Cr-Commit-Position: refs/heads/master@{#318786}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : #
Total comments: 7
Patch Set 5 : remove deps, add some missing EXTs #
Total comments: 4
Patch Set 6 : move dtor to Destroy(), fix binding #Patch Set 7 : #
Total comments: 1
Patch Set 8 : fix swapbuffers & overlay scheduling #
Total comments: 34
Patch Set 9 : #Patch Set 10 : switch to ScopedTextureBinder #Patch Set 11 : rebase #Patch Set 12 : synchronous swapbuffers #
Total comments: 4
Patch Set 13 : #Patch Set 14 : revert vsyncprovider change, change renderinghelper to not rely on the vsync cb #Patch Set 15 : add missing #if USE_OZONE #Patch Set 16 : dont need to modify vsync update path with synchronous swapbuffers now #
Messages
Total messages: 39 (6 generated)
achaulk@chromium.org changed reviewers: + dnicoara@chromium.org, spang@chromium.org
This should fix the VDA test breakage
https://codereview.chromium.org/938873002/diff/20001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/20001/ui/gl/gl_surface_ozone.c... ui/gl/gl_surface_ozone.cc:214: void BindFB() { I think this can be private, right? Also could you use long form of the name ("BindFramebuffer")? https://codereview.chromium.org/938873002/diff/20001/ui/gl/gl_surface_ozone.c... ui/gl/gl_surface_ozone.cc:252: ~GLSurfaceOzoneSurfacelessSurfaceImpl() override {} You'll need to cleanup all the allocated resources (fbo_, textures_, ...)
https://codereview.chromium.org/938873002/diff/20001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/20001/ui/gl/gl_surface_ozone.c... ui/gl/gl_surface_ozone.cc:214: void BindFB() { On 2015/02/19 00:10:13, dnicoara wrote: > I think this can be private, right? > > Also could you use long form of the name ("BindFramebuffer")? Done. https://codereview.chromium.org/938873002/diff/20001/ui/gl/gl_surface_ozone.c... ui/gl/gl_surface_ozone.cc:252: ~GLSurfaceOzoneSurfacelessSurfaceImpl() override {} On 2015/02/19 00:10:13, dnicoara wrote: > You'll need to cleanup all the allocated resources (fbo_, textures_, ...) Done.
lgtm
posciak@chromium.org changed reviewers: + posciak@chromium.org
content/common/gpu/media LGTM
achaulk@chromium.org changed reviewers: + piman@chromium.org
+piman for ui/gl, content
https://codereview.chromium.org/938873002/diff/60001/content/common/gpu/image... File content/common/gpu/image_transport_surface_linux.cc (right): https://codereview.chromium.org/938873002/diff/60001/content/common/gpu/image... content/common/gpu/image_transport_surface_linux.cc:17: #if USE_OZONE Don't you need to take the flag into account? Not all platforms can do surfaceless. Forcing it is not an option.
https://codereview.chromium.org/938873002/diff/60001/content/common/gpu/image... File content/common/gpu/image_transport_surface_linux.cc (right): https://codereview.chromium.org/938873002/diff/60001/content/common/gpu/image... content/common/gpu/image_transport_surface_linux.cc:17: #if USE_OZONE On 2015/02/19 00:42:04, spang wrote: > Don't you need to take the flag into account? Not all platforms can do > surfaceless. Forcing it is not an option. That's done inside ozone, if we don't support due to options it this will fail and we fall back
https://codereview.chromium.org/938873002/diff/60001/content/common/gpu/image... File content/common/gpu/image_transport_surface_linux.cc (right): https://codereview.chromium.org/938873002/diff/60001/content/common/gpu/image... content/common/gpu/image_transport_surface_linux.cc:17: #if USE_OZONE On 2015/02/19 00:43:07, achaulk wrote: > On 2015/02/19 00:42:04, spang wrote: > > Don't you need to take the flag into account? Not all platforms can do > > surfaceless. Forcing it is not an option. > > That's done inside ozone, if we don't support due to options it this will fail > and we fall back Ok, so this is "maybe create surfaceless, but I can accept a regular surface". Probably warrants a comment in the header in that case. Anyway, it should say #if defined(USE_OZONE) to match the usual style. https://codereview.chromium.org/938873002/diff/60001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/938873002/diff/60001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:710: tex_flip = 0; What if it's not surfaceless? https://codereview.chromium.org/938873002/diff/60001/ui/gl/DEPS File ui/gl/DEPS (right): https://codereview.chromium.org/938873002/diff/60001/ui/gl/DEPS#newcode6 ui/gl/DEPS:6: "+ui/ozone/gpu", Seems to be a cyclic dependency here.
https://codereview.chromium.org/938873002/diff/60001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/938873002/diff/60001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:710: tex_flip = 0; On 2015/02/19 00:49:26, spang wrote: > What if it's not surfaceless? Hmm, I could add a property to GLSurface to indicate if it is flipped or not, but otherwise we won't know https://codereview.chromium.org/938873002/diff/60001/ui/gl/DEPS File ui/gl/DEPS (right): https://codereview.chromium.org/938873002/diff/60001/ui/gl/DEPS#newcode6 ui/gl/DEPS:6: "+ui/ozone/gpu", On 2015/02/19 00:49:26, spang wrote: > Seems to be a cyclic dependency here. Ah, I'll pull in the bits of code I need, it's really just a GLImageEGL wrapper anyway
New patchsets have been uploaded after l-g-t-m from dnicoara@chromium.org,posciak@chromium.org
https://codereview.chromium.org/938873002/diff/80001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/80001/ui/gl/gl_surface_ozone.c... ui/gl/gl_surface_ozone.cc:248: glBindFramebufferEXT(GL_FRAMEBUFFER, 0); This is too late to do GL calls, the context is not current any more. I think you can do this at Destroy time, though it's worth it to DCHECK that the context that was passed to OnMakeCurrent is still current. https://codereview.chromium.org/938873002/diff/80001/ui/gl/gl_surface_ozone.c... ui/gl/gl_surface_ozone.cc:323: return NULL; nit: nullptr for consistency
On 2015/02/18 23:56:49, achaulk wrote: > This should fix the VDA test breakage I applied this together with https://codereview.chromium.org/936703002/, but I'm getting: [0220/130606:ERROR:native_display_delegate_dri.cc(199)] Not implemented reached in void ui::NativeDisplayDelegateDri::AddGraphicsDevice(const base::FilePath&) [0220/130606:WARNING:screen_manager.cc(78)] Display controller (crtc=18) already present. [0220/130606:ERROR:gl_context_egl.cc(125)] Could not make current. [0220/130606:FATAL:rendering_helper.cc(320)] Check failed: gl_context_->MakeCurrent(gl_surface_.get()).
On 2015/02/20 04:07:20, Pawel Osciak wrote: > On 2015/02/18 23:56:49, achaulk wrote: > > This should fix the VDA test breakage > > I applied this together with https://codereview.chromium.org/936703002/, but I'm > getting: > > [0220/130606:ERROR:native_display_delegate_dri.cc(199)] Not implemented reached > in void ui::NativeDisplayDelegateDri::AddGraphicsDevice(const base::FilePath&) > [0220/130606:WARNING:screen_manager.cc(78)] Display controller (crtc=18) already > present. > [0220/130606:ERROR:gl_context_egl.cc(125)] Could not make current. > [0220/130606:FATAL:rendering_helper.cc(320)] Check failed: > gl_context_->MakeCurrent(gl_surface_.get()). It's possible there's a bug in the init code, I haven't had a chance to test it on a device yet. I'm in MTV this week and loading builds from my laptop is awkward
There were a couple issues yeah, I was using the old EGL binding when it should have been the new DMA buffer one, and there were some things missing in the RenderingHelper init, and a deadlock from a dropped callback in ozone With this latest patch I now get 20/21 passing on link (Thumbnail/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 fails, looks unrelated to ozone, something about AtExitManager) https://codereview.chromium.org/938873002/diff/80001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/80001/ui/gl/gl_surface_ozone.c... ui/gl/gl_surface_ozone.cc:248: glBindFramebufferEXT(GL_FRAMEBUFFER, 0); On 2015/02/19 23:21:19, piman (Very slow to review) wrote: > This is too late to do GL calls, the context is not current any more. > I think you can do this at Destroy time, though it's worth it to DCHECK that the > context that was passed to OnMakeCurrent is still current. Done. https://codereview.chromium.org/938873002/diff/80001/ui/gl/gl_surface_ozone.c... ui/gl/gl_surface_ozone.cc:323: return NULL; On 2015/02/19 23:21:19, piman (Very slow to review) wrote: > nit: nullptr for consistency Done.
https://codereview.chromium.org/938873002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/938873002/diff/120001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:316: gl_surface_->Resize(platform_window_delegate_->GetSize()); You shouldn't access the |platform_window_delegate_| from here since this is called on the rendering thread while the |platform_window_delegate_| belongs on the main thread. I know this is a bit silly, but you'll likely need to stash the size similar to how |window_| is kept. https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:508: gl_surface_->Destroy(); This shouldn't be needed in here. Most implementations of GLSurface call this in their destructor. We should do the same in Ozone. https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:27: static void EmptyCallback() { Use base::DoNothing() https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:241: return SwapBuffersAsync(base::Bind(&EmptyCallback)); This is problematic since we're not guaranteeing that the PageFlip event completes. We probably need to implement this properly in the platform. https://codereview.chromium.org/938873002/diff/140001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_vsync_provider.cc (right): https://codereview.chromium.org/938873002/diff/140001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_vsync_provider.cc:27: callback.Run(base::TimeTicks(), Why do you need this?
https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:508: gl_surface_->Destroy(); On 2015/02/23 22:48:26, dnicoara wrote: > This shouldn't be needed in here. Most implementations of GLSurface call this in > their destructor. We should do the same in Ozone. That might cause issues with cleanup if there's no context https://codereview.chromium.org/938873002/diff/140001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_vsync_provider.cc (right): https://codereview.chromium.org/938873002/diff/140001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_vsync_provider.cc:27: callback.Run(base::TimeTicks(), On 2015/02/23 22:48:26, dnicoara wrote: > Why do you need this? Dropping the callback causes a deadlock
On 2015/02/23 20:43:10, achaulk wrote: > There were a couple issues yeah, I was using the old EGL binding when it should > have been the new DMA buffer one, and there were some things missing in the > RenderingHelper init, and a deadlock from a dropped callback in ozone > > With this latest patch I now get 20/21 passing on link > (Thumbnail/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 fails, looks > unrelated to ozone, something about AtExitManager) > This appears to have been caused by https://codereview.chromium.org/936703002/.
Was this compiled and tested for x86 when USE_OZONE is not defined and also for ARM? https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/imag... File content/common/gpu/image_transport_surface_linux.cc (right): https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/imag... content/common/gpu/image_transport_surface_linux.cc:20: if (!surface.get()) Just (!surface) should suffice. https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:316: gl_surface_->Resize(platform_window_delegate_->GetSize()); I think platform_window_delegate_ is under #if defined(USE_OZONE) ? https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:706: #if USE_OZONE #if defined(USE_OZONE) https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface.h File ui/gl/gl_surface.h (right): https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface.h#newc... ui/gl/gl_surface.h:159: #ifdef USE_OZONE #if defined for consistency. https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:205: class GL_EXPORT GLSurfaceOzoneSurfacelessSurfaceImpl Please define methods outside of the class body (http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts). https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:218: unsigned int GetBackingFrameBufferObject() override { return fbo_; } fbo_ is a GLuint and here we use unsigned int, perhaps we should use one or the other consistently. https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:227: if (!ResizePixmaps()) Please rename ResizePixmaps to something more descriptive of what it's actually doing (CreateAndBindPixmaps() perhaps?). The comment won't be needed then either. https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:248: GLSurfaceOzoneSurfaceless::SwapBuffersAsync(callback); This is difficult to read. Please split this statement. https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:250: BindFramebuffer(); Do we still want to do this if ret==false? https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:306: return true; Is this ok to succeed if !fbo_ ? https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:315: if (!image->Initialize(pixmap.get(), Please pass const scoped_refptr& instead of a pointer. https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:320: glBindTexture(GL_TEXTURE_2D, textures_[i]); Should we use ScopedTextureBinder instead? https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:328: GLuint textures_[2]; Please use std::vectors here and below.
https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:328: GLuint textures_[2]; On 2015/02/24 00:13:47, Pawel Osciak wrote: > Please use std::vectors here and below. Why? This is fine to me. Avoids extra indirections/allocations, and (IMO) produces clearer code.
https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:241: return SwapBuffersAsync(base::Bind(&EmptyCallback)); On 2015/02/23 22:48:26, dnicoara wrote: > This is problematic since we're not guaranteeing that the PageFlip event > completes. We probably need to implement this properly in the platform. Right, we need the ordering guarantee here (further GL commands will execute after the flip is done), as well as the backpressure.
On 2015/02/24 02:57:32, piman (Very slow to review) wrote: > https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.cc > File ui/gl/gl_surface_ozone.cc (right): > > https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... > ui/gl/gl_surface_ozone.cc:328: GLuint textures_[2]; > On 2015/02/24 00:13:47, Pawel Osciak wrote: > > Please use std::vectors here and below. > > Why? This is fine to me. Avoids extra indirections/allocations, and (IMO) > produces clearer code. To avoid hardcoding "2" everywhere in loops. We can always preallocate a vector. But I would be fine with arraysize() as well.
https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:258: if (fbo_) { Should probably reset these values and DCHECK in the destructor to make sure the resources are freed. https://codereview.chromium.org/938873002/diff/140001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_vsync_provider.cc (right): https://codereview.chromium.org/938873002/diff/140001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_vsync_provider.cc:27: callback.Run(base::TimeTicks(), On 2015/02/23 22:50:17, achaulk wrote: > On 2015/02/23 22:48:26, dnicoara wrote: > > Why do you need this? > > Dropping the callback causes a deadlock We should look into what's causing the deadlock. The VSyncProvider does not guarantee that this is called in every scenario.
https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/imag... File content/common/gpu/image_transport_surface_linux.cc (right): https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/imag... content/common/gpu/image_transport_surface_linux.cc:20: if (!surface.get()) On 2015/02/24 00:13:46, Pawel Osciak wrote: > Just (!surface) should suffice. Done. https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:316: gl_surface_->Resize(platform_window_delegate_->GetSize()); On 2015/02/24 00:13:47, Pawel Osciak wrote: > I think platform_window_delegate_ is under #if defined(USE_OZONE) ? Ah, yes it is https://codereview.chromium.org/938873002/diff/140001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:706: #if USE_OZONE On 2015/02/24 00:13:47, Pawel Osciak wrote: > #if defined(USE_OZONE) Done. https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:205: class GL_EXPORT GLSurfaceOzoneSurfacelessSurfaceImpl On 2015/02/24 00:13:47, Pawel Osciak wrote: > Please define methods outside of the class body > (http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts). The rest of this file uses the inline-class style https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:227: if (!ResizePixmaps()) On 2015/02/24 00:13:47, Pawel Osciak wrote: > Please rename ResizePixmaps to something more descriptive of what it's actually > doing (CreateAndBindPixmaps() perhaps?). The comment won't be needed then > either. Done. https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:241: return SwapBuffersAsync(base::Bind(&EmptyCallback)); On 2015/02/24 03:00:57, piman (Very slow to review) wrote: > On 2015/02/23 22:48:26, dnicoara wrote: > > This is problematic since we're not guaranteeing that the PageFlip event > > completes. We probably need to implement this properly in the platform. > > Right, we need the ordering guarantee here (further GL commands will execute > after the flip is done), as well as the backpressure. Can we block waiting for it? https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:248: GLSurfaceOzoneSurfaceless::SwapBuffersAsync(callback); On 2015/02/24 00:13:47, Pawel Osciak wrote: > This is difficult to read. Please split this statement. Done. https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:250: BindFramebuffer(); On 2015/02/24 00:13:47, Pawel Osciak wrote: > Do we still want to do this if ret==false? Hmm, probably not https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:306: return true; On 2015/02/24 00:13:47, Pawel Osciak wrote: > Is this ok to succeed if !fbo_ ? Resizing before binding should be fine - this will be executed on both paths anyway, so it will be guaranteed to happen at least from OnMakeCurrent https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:315: if (!image->Initialize(pixmap.get(), On 2015/02/24 00:13:47, Pawel Osciak wrote: > Please pass const scoped_refptr& instead of a pointer. Done. https://codereview.chromium.org/938873002/diff/140001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:320: glBindTexture(GL_TEXTURE_2D, textures_[i]); On 2015/02/24 00:13:47, Pawel Osciak wrote: > Should we use ScopedTextureBinder instead? It probably won't matter since this is at init, but sure
With lionel's recent ozone change we should be able to use the synchronous SwapBuffers here
One thing, and LGTM after that. https://codereview.chromium.org/938873002/diff/220001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/220001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:275: for (size_t i = 0; i < arraysize(textures_); i++) { nit: arraysize(images_) That said, you can also use the range loop syntax: for (auto image : images_) { if (image) image->Destroy(true); } https://codereview.chromium.org/938873002/diff/220001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:316: glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_); Use ScopedFrameBufferBinder here to make sure you're not clobbering state in the SwapBuffers case.
https://codereview.chromium.org/938873002/diff/220001/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/938873002/diff/220001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:275: for (size_t i = 0; i < arraysize(textures_); i++) { On 2015/02/27 21:53:26, piman (Very slow to review) wrote: > nit: arraysize(images_) > > That said, you can also use the range loop syntax: > for (auto image : images_) { > if (image) > image->Destroy(true); > } Oh I didn't know that worked on C-style arrays. That's handy https://codereview.chromium.org/938873002/diff/220001/ui/gl/gl_surface_ozone.... ui/gl/gl_surface_ozone.cc:316: glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_); On 2015/02/27 21:53:26, piman (Very slow to review) wrote: > Use ScopedFrameBufferBinder here to make sure you're not clobbering state in the > SwapBuffers case. This is intended to modify global state and leave our framebuffer bound. I suppose if somebody holds another scoped framebuffer across a swapbuffer and expects it to work it might blow up. Maybe we can get by with only binding it directly in MakeCurrent?
On Fri, Feb 27, 2015 at 2:00 PM, <achaulk@chromium.org> wrote: > > https://codereview.chromium.org/938873002/diff/220001/ui/ > gl/gl_surface_ozone.cc > File ui/gl/gl_surface_ozone.cc (right): > > https://codereview.chromium.org/938873002/diff/220001/ui/ > gl/gl_surface_ozone.cc#newcode275 > ui/gl/gl_surface_ozone.cc:275: for (size_t i = 0; i < > arraysize(textures_); i++) { > On 2015/02/27 21:53:26, piman (Very slow to review) wrote: > >> nit: arraysize(images_) >> > > That said, you can also use the range loop syntax: >> for (auto image : images_) { >> if (image) >> image->Destroy(true); >> } >> > > Oh I didn't know that worked on C-style arrays. That's handy > > https://codereview.chromium.org/938873002/diff/220001/ui/ > gl/gl_surface_ozone.cc#newcode316 > ui/gl/gl_surface_ozone.cc:316: glBindFramebufferEXT(GL_FRAMEBUFFER, > fbo_); > On 2015/02/27 21:53:26, piman (Very slow to review) wrote: > >> Use ScopedFrameBufferBinder here to make sure you're not clobbering >> > state in the > >> SwapBuffers case. >> > > This is intended to modify global state and leave our framebuffer bound. > I suppose if somebody holds another scoped framebuffer across a > swapbuffer and expects it to work it might blow up. Maybe we can get by > with only binding it directly in MakeCurrent? > From the point of view of the client, calling SwapBuffers should not change the FBO binding (and nothing guarantees that 0 is bound when SwapBuffers is called). Or for that matter MakeCurrent. Only the very first MakeCurrent is special. > > https://codereview.chromium.org/938873002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by achaulk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dnicoara@chromium.org, posciak@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/938873002/#ps300001 (title: "dont need to modify vsync update path with synchronous swapbuffers now")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/938873002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/e86e411c3038fd5be67438da29f0ff17828f226f Cr-Commit-Position: refs/heads/master@{#318786}
Message was sent while issue was closed.
On 2015/03/02 22:46:30, I haz the power (commit-bot) wrote: > Patchset 16 (id:??) landed as > https://crrev.com/e86e411c3038fd5be67438da29f0ff17828f226f > Cr-Commit-Position: refs/heads/master@{#318786} Broke clang ozone build for Chromecast. trybot not yet automatic, but can see failure at http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell/buil... FAILED: /b/build/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/ui/gl/gl.gl_surface_ozone.o.d -DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=223108 -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_OZONE=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP -DENABLE_WEBRTC=1 -DUSE_PROPRIETARY_CODECS -DENABLE_MPEG2TS_STREAM_PARSER -DENABLE_BROWSER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DDONT_EMBED_BUILD_METADATA -DLOG_DISABLED=0 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1 -DGL_IMPLEMENTATION -DSK_SUPPORT_GPU=1 -DSK_LEGACY_DRAWPICTURECALLBACK -DMESA_EGL_NO_X11_HEADERS -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DUSE_LIBPCI=1 -DUSE_NSS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -D_GLIBCXX_DEBUG=1 -Igen -I../../third_party/swiftshader/include -I../../third_party/khronos -I../../gpu -I../.. -I../../skia/config -I../../third_party/skia/src/core -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../skia/ext -I../../third_party/mesa/src/include -I../../third_party/icu/source/i18n -I../../third_party/icu/source/common -fstack-protector --param=ssp-buffer-size=4 -Werror -pthread -fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -Wno-reserved-user-defined-literal -Xclang -load -Xclang /b/build/slave/cast_shell/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-weak-ptr-factory-order -Xclang -plugin-arg-find-bad-constructs -Xclang strict-virtual-specifiers -fcolor-diagnostics -B/b/build/slave/cast_shell/build/src/third_party/binutils/Linux_x64/Release/bin -Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration -Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing -Wno-deprecated-register -Wno-inconsistent-missing-override -m64 -march=x86-64 -O0 -g -gdwarf-4 -funwind-tables -gsplit-dwarf -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare -std=gnu++11 -c ../../ui/gl/gl_surface_ozone.cc -o obj/ui/gl/gl.gl_surface_ozone.o ../../ui/gl/gl_surface_ozone.cc:284:9: error: [chromium-style] Classes that are ref-counted should have explicit destructors that are declared protected or private. class SurfaceImage : public GLImageLinuxDMABuffer { ^ ../../ui/gl/gl_surface_ozone.cc:284:24: note: [chromium-style] 'SurfaceImage' inherits from 'gfx::GLImageLinuxDMABuffer' here class SurfaceImage : public GLImageLinuxDMABuffer { ^ ../../ui/gl/gl_image_linux_dma_buffer.h:13:41: note: [chromium-style] 'GLImageLinuxDMABuffer' inherits from 'gfx::GLImageEGL' here class GL_EXPORT GLImageLinuxDMABuffer : public GLImageEGL { ^ ../../ui/gl/gl_image_egl.h:13:30: note: [chromium-style] 'GLImageEGL' inherits from 'gfx::GLImage' here class GL_EXPORT GLImageEGL : public GLImage { ^ ../../ui/gl/gl_image.h:20:27: note: [chromium-style] 'GLImage' inherits from 'base::RefCounted<GLImage>' here class GL_EXPORT GLImage : public base::RefCounted<GLImage> { ^ 1 error generated.
Message was sent while issue was closed.
On 2015/03/02 23:43:11, gunsch wrote: > On 2015/03/02 22:46:30, I haz the power (commit-bot) wrote: > > Patchset 16 (id:??) landed as > > https://crrev.com/e86e411c3038fd5be67438da29f0ff17828f226f > > Cr-Commit-Position: refs/heads/master@{#318786} > > Broke clang ozone build for Chromecast. trybot not yet automatic, but can see > failure at > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell/buil... > > > FAILED: /b/build/goma/gomacc > ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF > obj/ui/gl/gl.gl_surface_ozone.o.d -DV8_DEPRECATION_WARNINGS > -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=223108 > -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_OZONE=1 > -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN > -DENABLE_PRE_SYNC_BACKUP -DENABLE_WEBRTC=1 -DUSE_PROPRIETARY_CODECS > -DENABLE_MPEG2TS_STREAM_PARSER -DENABLE_BROWSER_CDMS > -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DDONT_EMBED_BUILD_METADATA > -DLOG_DISABLED=0 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 > -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 > -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_SPELLCHECK=1 > -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 > -DENABLE_SUPERVISED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1 > -DGL_IMPLEMENTATION -DSK_SUPPORT_GPU=1 -DSK_LEGACY_DRAWPICTURECALLBACK > -DMESA_EGL_NO_X11_HEADERS -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 > -DU_STATIC_IMPLEMENTATION -DUSE_LIBPCI=1 -DUSE_NSS=1 -D__STDC_CONSTANT_MACROS > -D__STDC_FORMAT_MACROS -DDYNAMIC_ANNOTATIONS_ENABLED=1 > -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -D_GLIBCXX_DEBUG=1 -Igen > -I../../third_party/swiftshader/include -I../../third_party/khronos -I../../gpu > -I../.. -I../../skia/config -I../../third_party/skia/src/core > -I../../third_party/skia/include/core -I../../third_party/skia/include/effects > -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu > -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops > -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports > -I../../third_party/skia/include/utils -I../../skia/ext > -I../../third_party/mesa/src/include -I../../third_party/icu/source/i18n > -I../../third_party/icu/source/common -fstack-protector > --param=ssp-buffer-size=4 -Werror -pthread -fno-strict-aliasing -Wall > -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe > -fPIC -Wno-reserved-user-defined-literal -Xclang -load -Xclang > /b/build/slave/cast_shell/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so > -Xclang -add-plugin -Xclang find-bad-constructs -Xclang > -plugin-arg-find-bad-constructs -Xclang check-weak-ptr-factory-order -Xclang > -plugin-arg-find-bad-constructs -Xclang strict-virtual-specifiers > -fcolor-diagnostics > -B/b/build/slave/cast_shell/build/src/third_party/binutils/Linux_x64/Release/bin > -Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration > -Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing > -Wno-deprecated-register -Wno-inconsistent-missing-override -m64 -march=x86-64 > -O0 -g -gdwarf-4 -funwind-tables -gsplit-dwarf -Wno-undefined-bool-conversion > -Wno-tautological-undefined-compare -fno-exceptions -fno-rtti > -fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare -std=gnu++11 > -c ../../ui/gl/gl_surface_ozone.cc -o obj/ui/gl/gl.gl_surface_ozone.o > ../../ui/gl/gl_surface_ozone.cc:284:9: error: [chromium-style] Classes that are > ref-counted should have explicit destructors that are declared protected or > private. > class SurfaceImage : public GLImageLinuxDMABuffer { > ^ > ../../ui/gl/gl_surface_ozone.cc:284:24: note: [chromium-style] 'SurfaceImage' > inherits from 'gfx::GLImageLinuxDMABuffer' here > class SurfaceImage : public GLImageLinuxDMABuffer { > ^ > ../../ui/gl/gl_image_linux_dma_buffer.h:13:41: note: [chromium-style] > 'GLImageLinuxDMABuffer' inherits from 'gfx::GLImageEGL' here > class GL_EXPORT GLImageLinuxDMABuffer : public GLImageEGL { > ^ > ../../ui/gl/gl_image_egl.h:13:30: note: [chromium-style] 'GLImageEGL' inherits > from 'gfx::GLImage' here > class GL_EXPORT GLImageEGL : public GLImage { > ^ > ../../ui/gl/gl_image.h:20:27: note: [chromium-style] 'GLImage' inherits from > 'base::RefCounted<GLImage>' here > class GL_EXPORT GLImage : public base::RefCounted<GLImage> { > ^ > 1 error generated. I mailed a fix at https://codereview.chromium.org/967343003 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
