Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23438002:
Adding functionality to paint and signal buffer swap for ozone surface factory. (Closed)
|
Created:
7 years, 3 months ago by dnicoara Modified:
4 years, 2 months ago CC:
chromium-reviews, rjkroege Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding functionality to paint and signal buffer swap for ozone surface factory.
OZONE event factory should be a bit more verbose on error.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220632
Patch Set 1 #Patch Set 2 : Forgot to include check #
Total comments: 10
Patch Set 3 : Address missing AcceleratedWidget and pure virtual comments #
Total comments: 6
Patch Set 4 : Using intptr_t for EGLNativeDisplayType on OZONE #
Total comments: 2
Patch Set 5 : Renamed SwapBuffers to SchedulePageFlip #
Total comments: 2
Patch Set 6 : Removing call to SchedulePageFlip in gl_surface_egl #
Total comments: 3
Messages
Total messages: 22 (1 generated)
https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... File ui/base/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... ui/base/ozone/surface_factory_ozone.h:8: #include <EGL/egl.h> this is not a good idea. https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... ui/base/ozone/surface_factory_ozone.h:14: class SkBitmap; SKia isn't in a name space? interesting. I learn something new everyday. https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... ui/base/ozone/surface_factory_ozone.h:44: virtual bool InitializeHardware() = 0; an enum would be preferable? And what does the status mean? :-) https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... ui/base/ozone/surface_factory_ozone.h:52: virtual EGLNativeDisplayType GetNativeDisplay() = 0; return an int. Or we need to modify some of the dfn's in khronos/egl.h to have some kind of typedef that does not require the EGL display stuff here. Also: please make these methods not pure virtual to avoid breaking down-stream implementors... We can make them pure once that the downstream implementations have had a chance to catch up. https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... ui/base/ozone/surface_factory_ozone.h:77: virtual bool SwapBuffers() = 0; this should take the accelerated widget as an argument in case there are multiple different widgets with different vsync properties? https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... ui/base/ozone/surface_factory_ozone.h:86: // Used to paint a bitmap directly to the surface. You are not using this method in this CL yes? Perhaps it belongs in a different CL where it is becoming a requirement?
https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... File ui/base/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... ui/base/ozone/surface_factory_ozone.h:44: virtual bool InitializeHardware() = 0; On 2013/08/26 21:48:08, rjkroege wrote: > an enum would be preferable? And what does the status mean? :-) I'm not sure what the status would be. I'm seeing this as a binary problem where the hardware is either initialized or not. Wouldn't a status be implementation dependent? https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... ui/base/ozone/surface_factory_ozone.h:52: virtual EGLNativeDisplayType GetNativeDisplay() = 0; On 2013/08/26 21:48:08, rjkroege wrote: > return an int. Or we need to modify some of the dfn's in khronos/egl.h to have > some kind of typedef that does not require the EGL display stuff here. > > Also: please make these methods not pure virtual to avoid breaking down-stream > implementors... We can make them pure once that the downstream implementations > have had a chance to catch up. Wouldn't this be platform dependent? I'm not sure how having it as an 'int' work without having multiple #ifdefs in gl_surface_egl to perform the appropriate casts/initializations. That said, Symbian is the only platform where it is defined as an 'int'. Others use a pointer type, so returning a 'void*' may be acceptable. The other alternative is to push eglGetDisplay into the surface factory, so we'd hide this all together. In this case I'd replace this function with "EGLDisplay GetEglDisplay()". https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... ui/base/ozone/surface_factory_ozone.h:77: virtual bool SwapBuffers() = 0; On 2013/08/26 21:48:08, rjkroege wrote: > this should take the accelerated widget as an argument in case there are > multiple different widgets with different vsync properties? Done. https://codereview.chromium.org/23438002/diff/5001/ui/base/ozone/surface_fact... ui/base/ozone/surface_factory_ozone.h:86: // Used to paint a bitmap directly to the surface. On 2013/08/26 21:48:08, rjkroege wrote: > You are not using this method in this CL yes? Perhaps it belongs in a different > CL where it is becoming a requirement? > Sure, I was thinking of having it in here to discuss the API, but we can do it later.
https://codereview.chromium.org/23438002/diff/11001/ui/base/ozone/surface_fac... File ui/base/ozone/surface_factory_ozone.cc (right): https://codereview.chromium.org/23438002/diff/11001/ui/base/ozone/surface_fac... ui/base/ozone/surface_factory_ozone.cc:21: virtual void* GetNativeDisplay() OVERRIDE { return 0; } this is also unnecessary. https://codereview.chromium.org/23438002/diff/11001/ui/base/ozone/surface_fac... ui/base/ozone/surface_factory_ozone.cc:33: virtual bool SwapBuffers(gfx::AcceleratedWidget w) OVERRIDE { return true; } this is unnecessary. https://codereview.chromium.org/23438002/diff/11001/ui/base/ozone/surface_fac... File ui/base/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/23438002/diff/11001/ui/base/ozone/surface_fac... ui/base/ozone/surface_factory_ozone.h:48: virtual void* GetNativeDisplay(); It isn't safe to assume that a void* fits in an int without changing EGLNativeDisplayType to intptr_t in third_party/khronos/EGL/eglplatform.h
https://codereview.chromium.org/23438002/diff/11001/ui/base/ozone/surface_fac... File ui/base/ozone/surface_factory_ozone.cc (right): https://codereview.chromium.org/23438002/diff/11001/ui/base/ozone/surface_fac... ui/base/ozone/surface_factory_ozone.cc:21: virtual void* GetNativeDisplay() OVERRIDE { return 0; } On 2013/08/27 16:03:42, rjkroege wrote: > this is also unnecessary. Done. https://codereview.chromium.org/23438002/diff/11001/ui/base/ozone/surface_fac... ui/base/ozone/surface_factory_ozone.cc:33: virtual bool SwapBuffers(gfx::AcceleratedWidget w) OVERRIDE { return true; } On 2013/08/27 16:03:42, rjkroege wrote: > this is unnecessary. Done. https://codereview.chromium.org/23438002/diff/11001/ui/base/ozone/surface_fac... File ui/base/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/23438002/diff/11001/ui/base/ozone/surface_fac... ui/base/ozone/surface_factory_ozone.h:48: virtual void* GetNativeDisplay(); On 2013/08/27 16:03:42, rjkroege wrote: > It isn't safe to assume that a void* fits in an int without changing > EGLNativeDisplayType to intptr_t in third_party/khronos/EGL/eglplatform.h > Updated the definition to intptr_t.
LGTM https://codereview.chromium.org/23438002/diff/17001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/23438002/diff/17001/ui/gl/gl_surface_egl.cc#n... ui/gl/gl_surface_egl.cc:372: if (!ui::SurfaceFactoryOzone::GetInstance()->SwapBuffers(window_)) { nit: rename per discussion.
Antoine, could you look over the third_party/khronos change. Thank you. On 2013/08/28 18:09:19, rjkroege wrote: > LGTM > > https://codereview.chromium.org/23438002/diff/17001/ui/gl/gl_surface_egl.cc > File ui/gl/gl_surface_egl.cc (right): > > https://codereview.chromium.org/23438002/diff/17001/ui/gl/gl_surface_egl.cc#n... > ui/gl/gl_surface_egl.cc:372: if > (!ui::SurfaceFactoryOzone::GetInstance()->SwapBuffers(window_)) { > nit: rename per discussion. Done
https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc#n... ui/gl/gl_surface_egl.cc:372: if (!ui::SurfaceFactoryOzone::GetInstance()->SchedulePageFlip(window_)) { This is weird. Why isn't it done by eglSwapBuffers?
On 2013/08/28 19:18:57, piman wrote: > https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc > File ui/gl/gl_surface_egl.cc (right): > > https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc#n... > ui/gl/gl_surface_egl.cc:372: if > (!ui::SurfaceFactoryOzone::GetInstance()->SchedulePageFlip(window_)) { > This is weird. Why isn't it done by eglSwapBuffers? I'm no expert in the matter, but I would think it decouples buffer management from interacting with the monitors. It may also be possible that the surface EGL is rendering to isn't intended for displaying to a monitor. In this case there is no page flip.
On 2013/08/28 20:07:19, dnicoara wrote: > On 2013/08/28 19:18:57, piman wrote: > > https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc > > File ui/gl/gl_surface_egl.cc (right): > > > > > https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc#n... > > ui/gl/gl_surface_egl.cc:372: if > > (!ui::SurfaceFactoryOzone::GetInstance()->SchedulePageFlip(window_)) { > > This is weird. Why isn't it done by eglSwapBuffers? > > I'm no expert in the matter, but I would think it decouples buffer management > from interacting with the monitors. You can't decouple it without adding logic higher up for throttling / buffer reuse. We rely on SwapBuffers blocking to throttle back the higher levels (to avoid silently skipping frames, or queuing an unbounded number of buffers, arbitrarily increasing memory and latency). > It may also be possible that the surface EGL > is rendering to isn't intended for displaying to a monitor. In this case there > is no page flip. NativeViewGLSurfaceEGL is meant to be "visible" to the "windowing system" opposed to being offscreen (what PbufferGLSurfaceEGL is).
On 2013/08/28 20:44:52, piman wrote: > On 2013/08/28 20:07:19, dnicoara wrote: > > On 2013/08/28 19:18:57, piman wrote: > > > https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc > > > File ui/gl/gl_surface_egl.cc (right): > > > > > > > > > https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc#n... > > > ui/gl/gl_surface_egl.cc:372: if > > > (!ui::SurfaceFactoryOzone::GetInstance()->SchedulePageFlip(window_)) { > > > This is weird. Why isn't it done by eglSwapBuffers? > > > > I'm no expert in the matter, but I would think it decouples buffer management > > from interacting with the monitors. > > You can't decouple it without adding logic higher up for throttling / buffer > reuse. We rely on SwapBuffers blocking to throttle back the higher levels (to > avoid silently skipping frames, or queuing an unbounded number of buffers, > arbitrarily increasing memory and latency). > > > It may also be possible that the surface EGL > > is rendering to isn't intended for displaying to a monitor. In this case there > > is no page flip. > > NativeViewGLSurfaceEGL is meant to be "visible" to the "windowing system" > opposed to being offscreen (what PbufferGLSurfaceEGL is). Sorry, maybe I should have mentioned this before, this is meant for cases where you might be dealing directly with the DRM/KMS stack where there is no window manager (such as X) to provide this functionality. In this case OZONE would provide some of this functionality.
On Wed, Aug 28, 2013 at 2:19 PM, <dnicoara@chromium.org> wrote: > On 2013/08/28 20:44:52, piman wrote: > >> On 2013/08/28 20:07:19, dnicoara wrote: >> > On 2013/08/28 19:18:57, piman wrote: >> > > >> > https://codereview.chromium.**org/23438002/diff/22001/ui/gl/** > gl_surface_egl.cc<https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc> > >> > > File ui/gl/gl_surface_egl.cc (right): >> > > >> > > >> > >> > > https://codereview.chromium.**org/23438002/diff/22001/ui/gl/** > gl_surface_egl.cc#newcode372<https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc#newcode372> > >> > > ui/gl/gl_surface_egl.cc:372: if >> > > (!ui::SurfaceFactoryOzone::**GetInstance()->**SchedulePageFlip(window_)) >> { >> > > This is weird. Why isn't it done by eglSwapBuffers? >> > >> > I'm no expert in the matter, but I would think it decouples buffer >> > management > >> > from interacting with the monitors. >> > > You can't decouple it without adding logic higher up for throttling / >> buffer >> reuse. We rely on SwapBuffers blocking to throttle back the higher levels >> (to >> avoid silently skipping frames, or queuing an unbounded number of buffers, >> arbitrarily increasing memory and latency). >> > > > It may also be possible that the surface EGL >> > is rendering to isn't intended for displaying to a monitor. In this case >> > there > >> > is no page flip. >> > > NativeViewGLSurfaceEGL is meant to be "visible" to the "windowing system" >> opposed to being offscreen (what PbufferGLSurfaceEGL is). >> > > Sorry, maybe I should have mentioned this before, this is meant for cases > where > you might be dealing directly with the DRM/KMS stack where there is no > window > manager (such as X) to provide this functionality. In the DRM/KMS stack, the kernel is what passes for the "windowing system", in that it's what presents to the screen. > In this case OZONE would > provide some of this functionality. > http://www.khronos.org/registry/egl/sdk/docs/man/xhtml/eglSwapBuffers.html "Subsequent client API commands may be issued on that context immediately after calling eglSwapBuffers, but are not executed until the buffer exchange is completed." It's the responsibility of eglSwapBuffers to queue commands or block until the buffers have been swapped. The driver is broken if it doesn't do it. > > https://codereview.chromium.**org/23438002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi. In my Ozone Wayland implementation, I need to ask for the EGL library a
different configuration than the one provided in there. Something like this:
@@ -134,7 +132,7 @@ bool GLSurfaceEGL::InitializeOneOff() {
EGL_GREEN_SIZE, 8,
EGL_RED_SIZE, 8,
EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
- EGL_SURFACE_TYPE, EGL_WINDOW_BIT | EGL_PBUFFER_BIT,
+ EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
hould we build a method for SurfaceFactoryOzone for that as well?
(BTW @dnicoara I think you might be interested on checking the Wayland
implementation. We're in the process of opening it, but if you're curious you
can send me via email your github username and I give you the perms)
https://codereview.chromium.org/23438002/diff/17001/third_party/khronos/EGL/e...
File third_party/khronos/EGL/eglplatform.h (right):
https://codereview.chromium.org/23438002/diff/17001/third_party/khronos/EGL/e...
third_party/khronos/EGL/eglplatform.h:99: typedef intptr_t EGLNativeDisplayType;
changes in khronos EGL are not causing any effect AFAIK and that's a known bug:
https://code.google.com/p/chromium/issues/detail?id=266310
@dnicoara, you confirm that your changes here actually has some effect?
On 2013/08/29 18:09:34, vignatti wrote:
> Hi. In my Ozone Wayland implementation, I need to ask for the EGL library a
> different configuration than the one provided in there. Something like this:
>
> @@ -134,7 +132,7 @@ bool GLSurfaceEGL::InitializeOneOff() {
> EGL_GREEN_SIZE, 8,
> EGL_RED_SIZE, 8,
> EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> - EGL_SURFACE_TYPE, EGL_WINDOW_BIT | EGL_PBUFFER_BIT,
> + EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
>
> hould we build a method for SurfaceFactoryOzone for that as well?
>
I've spoken with @rjkroege about this. It is likely that the OZONE surface
factory will provide a call to get the EGL config attributes.
> (BTW @dnicoara I think you might be interested on checking the Wayland
> implementation. We're in the process of opening it, but if you're curious you
> can send me via email your github username and I give you the perms)
>
>
https://codereview.chromium.org/23438002/diff/17001/third_party/khronos/EGL/e...
> File third_party/khronos/EGL/eglplatform.h (right):
>
>
https://codereview.chromium.org/23438002/diff/17001/third_party/khronos/EGL/e...
> third_party/khronos/EGL/eglplatform.h:99: typedef intptr_t
EGLNativeDisplayType;
> changes in khronos EGL are not causing any effect AFAIK and that's a known
bug:
> https://code.google.com/p/chromium/issues/detail?id=266310
>
> @dnicoara, you confirm that your changes here actually has some effect?
Right now they don't due to include ordering. I've spoken with @rjkroege about
it and the include ordering will be fixed at some point.
On 2013/08/29 19:52:33, dnicoara wrote: > On 2013/08/29 18:09:34, vignatti wrote: > > > https://codereview.chromium.org/23438002/diff/17001/third_party/khronos/EGL/e... > > File third_party/khronos/EGL/eglplatform.h (right): > > > > > https://codereview.chromium.org/23438002/diff/17001/third_party/khronos/EGL/e... > > third_party/khronos/EGL/eglplatform.h:99: typedef intptr_t > EGLNativeDisplayType; > > changes in khronos EGL are not causing any effect AFAIK and that's a known > bug: > > https://code.google.com/p/chromium/issues/detail?id=266310 > > > > @dnicoara, you confirm that your changes here actually has some effect? > > Right now they don't due to include ordering. I've spoken with @rjkroege about > it and the include ordering will be fixed at some point. ok, thanks for clarifying. It's just funny that you're changing that EGL header now but in practice nothing is happening... maybe we need to at least make both khronos and Mesa equivalent on Ozone then?
Sorry about that. I've had a long discussion with @rjkroege and thanks to him I understand what is expected and how to proceed. https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/23438002/diff/22001/ui/gl/gl_surface_egl.cc#n... ui/gl/gl_surface_egl.cc:372: if (!ui::SurfaceFactoryOzone::GetInstance()->SchedulePageFlip(window_)) { On 2013/08/28 19:18:57, piman wrote: > This is weird. Why isn't it done by eglSwapBuffers? Removed.
lgtm
https://codereview.chromium.org/23438002/diff/36001/ui/base/ozone/surface_fac... File ui/base/ozone/surface_factory_ozone.h (right): https://codereview.chromium.org/23438002/diff/36001/ui/base/ozone/surface_fac... ui/base/ozone/surface_factory_ozone.h:77: // Called after the appropriate GL swap buffers command. Used if extra work I realize it's been a bit of contentious discussion but... Given that no one calls this at present and it's not yet clear to me if we _must_ have this method, perhaps you could not include in this CL? https://codereview.chromium.org/23438002/diff/36001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/23438002/diff/36001/ui/gl/gl_surface_egl.cc#n... ui/gl/gl_surface_egl.cc:119: surface_factory->GetNativeDisplay()); it would be nice to have the stub GetNativeDisplay() return the int equivalent to EGL_DEFAULT_DISPLAY?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dnicoara@chromium.org/23438002/36001
Message was sent while issue was closed.
Change committed as 220632
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
Not sure if we can still change this, but: https://chromiumcodereview.appspot.com/23438002/diff/36001/third_party/khrono... File third_party/khronos/EGL/eglplatform.h (right): https://chromiumcodereview.appspot.com/23438002/diff/36001/third_party/khrono... third_party/khronos/EGL/eglplatform.h:99: typedef intptr_t EGLNativeDisplayType; why is this intptr_t instead of void*? The typedefs for all other platforms are pointers, so ozone being the odd system out is a bit inconvenient (eg you can initialize an EGLNativeDisplayType with nullptr everywhere but ozone, you can make an invalid id with reinterpret_cast<EGLNativeDisplayType>(1) everywhere but ozone, etc) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
