Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11861020:
Aura: Browser-side changes for Composite-To-Mailbox (Closed)
|
Created:
7 years, 11 months ago by no sievers Modified:
7 years, 9 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, jam, penghuang+watch_chromium.org, apatrick_chromium, joi+watch-content_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, James Su, piman, aelias_OOO_until_Jul13, Sami, klobag.chromium, jamesr, nduca, brianderson, slavi Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAura: Browser-side changes for Composite-To-Mailbox
This adds browser-side plumbing for receiving output frames from the renderer directly
(with the CompositorFrame message) using the GL_CHROMIUM_texture_mailbox extension,
which essentially bypasses the GPU thread in the swap logic.
TODO:
- Avoid leaking textures in the mailbox during shutdown races.
- Create a different kind of transport surface stub on the GPU thread in this mode.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185468
Patch Set 1 #
Total comments: 24
Patch Set 2 : #Patch Set 3 : add unit tests #Patch Set 4 : #
Total comments: 13
Patch Set 5 : address comments #Patch Set 6 : browser-side changes only #
Total comments: 3
Patch Set 7 : add missing return statement #
Total comments: 1
Patch Set 8 : #Messages
Total messages: 26 (0 generated)
This is what I have so far and.. well it runs, But the changes in gl_renderer.cc are probably pretty questionable still at this stage. The rest is mostly plumbing and probably close to reviewable. other TODOs: avoid mailbox leaks during shutdown, unit tests...
some quick thoughts https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode218 cc/gl_renderer.cc:218: if (m_renderToMailbox) { if (!m_renderToMailbox) return; https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode224 cc/gl_renderer.cc:224: for (; it != m_pendingFrames.end(); it++) { shouldn't the frame always be the .front() here, similar to the assumption in the else{} below? https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode231 cc/gl_renderer.cc:231: DCHECK(texture.textureId); if you like, i think you could create a texture id for use here, instead of storing it, which would be cheap as it just creates a client id. https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.h File cc/gl_renderer.h (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.h#newcode238 cc/gl_renderer.h:238: struct MailboxTexture considering that we're going to have a TextureMailbox class in cc/ soon (https://codereview.chromium.org/11888010/) I think using a different or more verbose name here would be super nice. TransferableFrame?
https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode224 cc/gl_renderer.cc:224: for (; it != m_pendingFrames.end(); it++) { On 2013/01/18 01:01:41, danakj wrote: > shouldn't the frame always be the .front() here, similar to the assumption in > the else{} below? The one exception is skipped frames. When RWHV decides that it does not want to display the current frame (for example because it has the wrong size), then it will bounce the latest frame back that it received (instead of the older one it already had). And since we might have already swapped another frame, it's also not as easy as just grabbing either front() or back(). We could also send the size back with the ACK, then we would have to do less tracking here. I guess I was really only doing this to get the size of the texture :)
I think a lot of the new logic in gl_renderer could be extracted to the output surface, basically adding entry points such as - Swap/PostSubBuffer - Ensure/Discard backbuffer - Bind backbuffer inside OutputSurface, we would have 2 implementations, one that goes directly to the context, and one that goes to the new path. For the ack on old path, the OutputSurface instead of the GLRenderer would be implementing the WebGraphicsSwapBuffersCompleteCallbackCHROMIUM and could feed it directly to the LayerTreeHostImpl, through the OutputSurfaceClient. I think conceptually that separates nicely the rendering logic (in Renderer) from the presentation logic (in OutputSurface) WDYT? https://codereview.chromium.org/11861020/diff/1/cc/compositor_frame_ack.h File cc/compositor_frame_ack.h (right): https://codereview.chromium.org/11861020/diff/1/cc/compositor_frame_ack.h#new... cc/compositor_frame_ack.h:20: uint32 sync_point; I wonder if we need to do like for the CompositorFrame, and have a DelegatedFrameAck and a GLFrameAck https://codereview.chromium.org/11861020/diff/1/cc/gl_frame_data.h File cc/gl_frame_data.h (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_frame_data.h#newcode24 cc/gl_frame_data.h:24: gfx::Size size; I think this is redundant with the metadata's viewport_size https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode160 cc/gl_renderer.cc:160: DCHECK(m_pendingFrames.size() == 1); What ensures that this doesn't get deleted while a new frame is in flight (the ack hasn't been received yet)? https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode1376 cc/gl_renderer.cc:1376: m_context->flush(); We definitely also want to support the partial swap case. I think it's just a matter of passing the dirty rect into the GLFrame. https://codereview.chromium.org/11861020/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/11861020/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_impl.cc:2331: cc::switches::kCompositeToMailbox); This is a bit of a hack. At the call site, depending on the path, the renderer id got shoved into the gpu id, and then, based on the command-line flag, we decide how to interpret it. How about having 2 methods AcknowledgeBufferPresentToGpu vs AcknowledgeBufferPresentToRenderer, the latter one doesn't even need the route id or the host id (they're members of this class, you passed them up there in OnSwapCompositorFrame) https://codereview.chromium.org/11861020/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/11861020/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_aura.cc:891: cc::switches::kCompositeToMailbox); Do we need to test the command line here? Should we instead rely on whether frame.gl_frame_data is set? We'll have another path for the delegated_frame_data, and it seems superfluous to test the command line instead of "do what's necessary based on the data we got" https://codereview.chromium.org/11861020/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_aura.cc:903: mailbox_name.assign( nit: you can just use the constructor with the same parameters
https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode219 cc/gl_renderer.cc:219: onSwapBuffersComplete(); Should we consume the texture and insert the sync point wait before this callback? I'm not sure if the delegate could do something with the context that would get executed too soon. https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode1376 cc/gl_renderer.cc:1376: m_context->flush(); Is it necessary to flush after inserting a sync point? If yes, then it seems like the renderer could deadlock the browser by sending it a sync point without ever flushing. https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode1495 cc/gl_renderer.cc:1495: m_context->texParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); Nit: could use texParameteri consistently.
https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode1376 cc/gl_renderer.cc:1376: m_context->flush(); On 2013/01/25 23:17:27, Sami wrote: > Is it necessary to flush after inserting a sync point? The flush isn't necessary to ensure the sync point makes it through - insertSyncPoint returns once the sync point is in the queue to be processed (in order wrt the other commands). It is necessary however to ensure the results of the previous commands is visible by the other contexts. > If yes, then it seems > like the renderer could deadlock the browser by sending it a sync point without > ever flushing. It is possible for a malicious renderer to deadlock the browser process, but it's quite hard to do it accidentally: essentially the renderer has to deadlock its command buffer processing, by having to guess the upcoming syncpoint id, and rely on a race on the GPU process side, by doing waitSyncPoint(x); insertSyncPoint();, where x is the sync point that will be returned by the insertSyncPoint. After that, the renderer could send x to the browser and deadlock it too. We could make the ids unguessable (crypto-random), but other attack could do this, e.g. by making the command buffer deadlock (possible by adding a jump command that makes it spin in a loop). That said, all the renderer can do is deadlock the browser, which is a low-value attack. If we want to solve it, we need to give up the latency/pipelining benefits of this CL, i.e. using signalSyncPoint in the browser, which would only call back once the sync point has passed on the GPU process. Another alternative is to keep track of the renderers we may be waiting on, and kill them if the GPU process doesn't make progress from the pov of the browser (after a timeout), which will kill their contexts and release the sync points.
https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode1376 cc/gl_renderer.cc:1376: m_context->flush(); On 2013/01/25 23:44:56, piman wrote: > It is possible for a malicious renderer to deadlock the browser process, but > it's quite hard to do it accidentally: essentially the renderer has to deadlock > its command buffer processing, by having to guess the upcoming syncpoint id, and > rely on a race on the GPU process side, by doing waitSyncPoint(x); > insertSyncPoint();, where x is the sync point that will be returned by the > insertSyncPoint. After that, the renderer could send x to the browser and > deadlock it too. > We could make the ids unguessable (crypto-random), but other attack could do > this, e.g. by making the command buffer deadlock (possible by adding a jump > command that makes it spin in a loop). > That said, all the renderer can do is deadlock the browser, which is a low-value > attack. If we want to solve it, we need to give up the latency/pipelining > benefits of this CL, i.e. using signalSyncPoint in the browser, which would only > call back once the sync point has passed on the GPU process. > Another alternative is to keep track of the renderers we may be waiting on, and > kill them if the GPU process doesn't make progress from the pov of the browser > (after a timeout), which will kill their contexts and release the sync points. Thanks for the explanation. I was just wondering whether we should make creating a sync point perform an implicit flush, but it sounds like that wouldn't completely eliminate the possibility of a deadlock. Perhaps it would be best to have a timeout on the wait like you said. Seems a little out of scope of this patch though.
https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode1376 cc/gl_renderer.cc:1376: m_context->flush(); On 2013/01/26 00:30:27, Sami wrote: > On 2013/01/25 23:44:56, piman wrote: > > It is possible for a malicious renderer to deadlock the browser process, but > > it's quite hard to do it accidentally: essentially the renderer has to > deadlock > > its command buffer processing, by having to guess the upcoming syncpoint id, > and > > rely on a race on the GPU process side, by doing waitSyncPoint(x); > > insertSyncPoint();, where x is the sync point that will be returned by the > > insertSyncPoint. After that, the renderer could send x to the browser and > > deadlock it too. > > We could make the ids unguessable (crypto-random), but other attack could do > > this, e.g. by making the command buffer deadlock (possible by adding a jump > > command that makes it spin in a loop). > > That said, all the renderer can do is deadlock the browser, which is a > low-value > > attack. If we want to solve it, we need to give up the latency/pipelining > > benefits of this CL, i.e. using signalSyncPoint in the browser, which would > only > > call back once the sync point has passed on the GPU process. > > Another alternative is to keep track of the renderers we may be waiting on, > and > > kill them if the GPU process doesn't make progress from the pov of the browser > > (after a timeout), which will kill their contexts and release the sync points. > > Thanks for the explanation. I was just wondering whether we should make creating > a sync point perform an implicit flush, It does an implicit shallow flush FWIW, to ensure ordering within the same channel. It's not enough to ensure consistency across contexts though, and I'm wondering if we should possibly either: - do a full flush in insertSyncPoint - or make the shallow flush "lazy", i.e. a noop if we just flushed > but it sounds like that wouldn't > completely eliminate the possibility of a deadlock. Perhaps it would be best to > have a timeout on the wait like you said. Seems a little out of scope of this > patch though. Right, I talked to security folks, and they're ok even with a reasonably long timeout (15-30s). Definitely out-of-scope for this CL.
https://codereview.chromium.org/11861020/diff/1/cc/compositor_frame_ack.h File cc/compositor_frame_ack.h (right): https://codereview.chromium.org/11861020/diff/1/cc/compositor_frame_ack.h#new... cc/compositor_frame_ack.h:20: uint32 sync_point; On 2013/01/18 02:14:11, piman wrote: > I wonder if we need to do like for the CompositorFrame, and have a > DelegatedFrameAck and a GLFrameAck I changed it to use GLFrameData also, since it uses the same 3 elements now (mailbox, sync point, size) for the ACK. https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.cc#newcode1495 cc/gl_renderer.cc:1495: m_context->texParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); On 2013/01/25 23:17:27, Sami wrote: > Nit: could use texParameteri consistently. Done. https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.h File cc/gl_renderer.h (right): https://codereview.chromium.org/11861020/diff/1/cc/gl_renderer.h#newcode238 cc/gl_renderer.h:238: struct MailboxTexture On 2013/01/18 01:01:41, danakj wrote: > considering that we're going to have a TextureMailbox class in cc/ soon > (https://codereview.chromium.org/11888010/) I think using a different or more > verbose name here would be super nice. > > TransferableFrame? Done. https://codereview.chromium.org/11861020/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/11861020/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_impl.cc:2331: cc::switches::kCompositeToMailbox); Note that the method needs to be static though. But I've cleaned this up regardless. This also makes it easier to return the size of the texture being returned, which allows me to get rid of the ack tracking in the renderer. https://codereview.chromium.org/11861020/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/11861020/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_aura.cc:891: cc::switches::kCompositeToMailbox); On 2013/01/18 02:14:11, piman wrote: > Do we need to test the command line here? Should we instead rely on whether > frame.gl_frame_data is set? We'll have another path for the > delegated_frame_data, and it seems superfluous to test the command line instead > of "do what's necessary based on the data we got" Done. https://codereview.chromium.org/11861020/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_aura.cc:903: mailbox_name.assign( On 2013/01/18 02:14:11, piman wrote: > nit: you can just use the constructor with the same parameters Done.
On 2013/01/18 02:14:10, piman wrote: > I think a lot of the new logic in gl_renderer could be extracted to the output > surface, basically adding entry points such as > - Swap/PostSubBuffer > - Ensure/Discard backbuffer > - Bind backbuffer > > inside OutputSurface, we would have 2 implementations, one that goes directly to > the context, and one that goes to the new path. > > For the ack on old path, the OutputSurface instead of the GLRenderer would be > implementing the WebGraphicsSwapBuffersCompleteCallbackCHROMIUM and could feed > it directly to the LayerTreeHostImpl, through the OutputSurfaceClient. > I've done this... for the most part at least. I'm not too happy with adding the OutputSurface definitions in the base class, but it's a little tricky as derived classes (for example BrowserCompositorOutputSurface or test classes) need to be going through this default codepath. In order to avoid routing the swap ack through the GLRenderer, I'd probably want to move the usingSwapCompleteCallback capability to the output surface, which would need a bit more plumbing changes: We'd need a good place in the output surface to query extensions. Since Context3D() cannot be called from the constructor, I could require a context to be passed into the constructor (which from taking a quick glance looked like would pretty much work with all derived versions incl. ones used for testing.)
It looks like you and Slavi have done the many of the same things to cc::OutputSurface (see https://codereview.chromium.org/12041062/)
On 2013/02/06 23:41:48, jamesr wrote: > It looks like you and Slavi have done the many of the same things to > cc::OutputSurface (see https://codereview.chromium.org/12041062/) Ok, that's good then. And actually that answers my other question, because he did this: OutputSurface(scoped_ptr<WebKit::WebGraphicsContext3D> context3d); That means I'll just move RendererCapabilities.usingPartialSwap to OutputSurface.capabilities_ and this will also take care of this TODO in my patch (because I can check the extension from the constructor then): void OutputSurface::EnsureBackbuffer() { 16 // TODO: check extension
I will also look into adding unit tests.
On 2013/02/06 23:48:40, Daniel Sievers wrote: > I will also look into adding unit tests. The kind of unit tests I would like to write for this are gl_renderer_unittests that make sure we call the right functions in mailbox and normal mode (i.e. don't call framebuffer-related functions in WGC3D directly). This is hard with this hierarchy: MailboxOutputSurface derives from CompositorOutputSurface derives from OutputSurface. In the current tests FakeOutputSurface derives from OutputSurface directly to bypass the code in CompositorOutputSurface (IPC, vsync and thread-prio related stuff). I'm wondering if the code in CompositorOutputSurface should go into a separate helper class (compare to ImageTransportSurface and ImageTransportHelper on the gpu thread). Sound good?
On Wed, Feb 6, 2013 at 4:29 PM, <sievers@chromium.org> wrote: > On 2013/02/06 23:48:40, Daniel Sievers wrote: > >> I will also look into adding unit tests. >> > > The kind of unit tests I would like to write for this are > gl_renderer_unittests > that make sure we call the right functions in mailbox and normal mode (i.e. > don't call framebuffer-related functions in WGC3D directly). > > This is hard with this hierarchy: MailboxOutputSurface derives from > CompositorOutputSurface derives from OutputSurface. > > In the current tests FakeOutputSurface derives from OutputSurface directly > to > bypass the code in CompositorOutputSurface (IPC, vsync and thread-prio > related > stuff). > > I'm wondering if the code in CompositorOutputSurface should go into a > separate > helper class (compare to ImageTransportSurface and ImageTransportHelper on > the > gpu thread). > > Sound good? > Composition > inheritance, SG. > > > > https://codereview.chromium.**org/11861020/<https://codereview.chromium.org/1... >
Added unit tests. I tried not routing the CompositorFrameAck through GLRenderer but it turned out a bit messy and I was afraid I'd unnecessarily break something in this patch (it would have to be moved for all renderer types and right now it's the RendererClient that gets the callback and the logic to configure it in all cases is a bit interesting and spread, i.e. child compositor/threaded or not/depending on GL extension). https://codereview.chromium.org/11861020/diff/25001/cc/output_surface.cc File cc/output_surface.cc (right): https://codereview.chromium.org/11861020/diff/25001/cc/output_surface.cc#newc... cc/output_surface.cc:58: DCHECK(context3d_); Really it should query the extension here like it did before my patch. But I'd have to do it lazily here or so, because the context3d passed into the constructor is not initialized yet and cannot be used there.
Looking good... I think your TODOs are still relevant, especially the leak possibility (e.g. renderer was killed, we sent an Ack with a mailbox, no-one will clean up) - and I don't have a great solution right now (we'll have the same problem with ÜC). I suggest splitting off the OutputSurface refactor (with the tests, thanks!), which is good on its own. https://codereview.chromium.org/11861020/diff/25001/cc/output_surface.cc File cc/output_surface.cc (right): https://codereview.chromium.org/11861020/diff/25001/cc/output_surface.cc#newc... cc/output_surface.cc:58: DCHECK(context3d_); On 2013/02/27 00:12:30, Daniel Sievers wrote: > Really it should query the extension here like it did before my patch. But I'd > have to do it lazily here or so, because the context3d passed into the > constructor is not initialized yet and cannot be used there. You can check in BindToClient, which has to be called before DiscardBackbuffer (and friends). https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_impl.cc:1540: view_->OnSwapCompositorFrame(frame, process_->GetID(), routing_id_); We need to send an Ack if we have no view, otherwise the renderer will block. https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:345: } I /think/ those 4 functions could be factored into 2 (ie merging the skipped vs success cases), and factor some of the logic into the callers (by having the BuffersSwapped failure case call the callback). Essentially they all want to send a message one way or another, with: - a mailbox name - an optional sync point Both 'success' functions call Produce on the texture and InsertSyncPoint to derive the mailbox name and sync point. Both 'skip' functions use the mailbox name from the incoming frame, and a 0 sync point. So you could have the logic to Produce and InsertSyncPoint (success case) in SwapBuffersCompleted (no compositor case) and an intermediate callback (compositor case), and have SwapBuffersPrepare directly call the ack_callback with the incoming frame mailbox name (and 0 sync point). The advantage is that it would make it a lot more obvious that both paths do the same thing. https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:1212: const scoped_refptr<ui::Texture>& texture_to_return) { nit:indent https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:1275: DCHECK(frame.gl_frame_data->sync_point); note: all these DCHECKs are based on untrusted data coming from the renderer. It'd be better probably to skip the frame. https://codereview.chromium.org/11861020/diff/25001/content/renderer/gpu/mail... File content/renderer/gpu/mailbox_output_surface.cc (right): https://codereview.chromium.org/11861020/diff/25001/content/renderer/gpu/mail... content/renderer/gpu/mailbox_output_surface.cc:78: } I think it would be good to destroy the FBO here. Some drivers (especially tilers) keep heavy associated data with it. On top of that, if it's not current, per spec it'll keep an internal reference to the texture (you can avoid the latter part by making it current) before destroying the texture).
https://codereview.chromium.org/11861020/diff/25001/cc/output_surface.cc File cc/output_surface.cc (right): https://codereview.chromium.org/11861020/diff/25001/cc/output_surface.cc#newc... cc/output_surface.cc:58: DCHECK(context3d_); On 2013/02/27 01:07:13, piman wrote: > On 2013/02/27 00:12:30, Daniel Sievers wrote: > > Really it should query the extension here like it did before my patch. But I'd > > have to do it lazily here or so, because the context3d passed into the > > constructor is not initialized yet and cannot be used there. > > You can check in BindToClient, which has to be called before DiscardBackbuffer > (and friends). Done. https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_impl.cc:1540: view_->OnSwapCompositorFrame(frame, process_->GetID(), routing_id_); On 2013/02/27 01:07:13, piman wrote: > We need to send an Ack if we have no view, otherwise the renderer will block. Done. https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:345: } On 2013/02/27 01:07:13, piman wrote: > I /think/ those 4 functions could be factored into 2 (ie merging the skipped vs > success cases), and factor some of the logic into the callers (by having the > BuffersSwapped failure case call the callback). > > Essentially they all want to send a message one way or another, with: > - a mailbox name > - an optional sync point > > Both 'success' functions call Produce on the texture and InsertSyncPoint to > derive the mailbox name and sync point. Both 'skip' functions use the mailbox > name from the incoming frame, and a 0 sync point. > > So you could have the logic to Produce and InsertSyncPoint (success case) in > SwapBuffersCompleted (no compositor case) and an intermediate callback > (compositor case), and have SwapBuffersPrepare directly call the ack_callback > with the incoming frame mailbox name (and 0 sync point). The advantage is that > it would make it a lot more obvious that both paths do the same thing. Done. https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:1212: const scoped_refptr<ui::Texture>& texture_to_return) { On 2013/02/27 01:07:13, piman wrote: > nit:indent Done. https://codereview.chromium.org/11861020/diff/25001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:1275: DCHECK(frame.gl_frame_data->sync_point); On 2013/02/27 01:07:13, piman wrote: > note: all these DCHECKs are based on untrusted data coming from the renderer. > It'd be better probably to skip the frame. Done. https://codereview.chromium.org/11861020/diff/25001/content/renderer/gpu/mail... File content/renderer/gpu/mailbox_output_surface.cc (right): https://codereview.chromium.org/11861020/diff/25001/content/renderer/gpu/mail... content/renderer/gpu/mailbox_output_surface.cc:78: } On 2013/02/27 01:07:13, piman wrote: > I think it would be good to destroy the FBO here. Some drivers (especially > tilers) keep heavy associated data with it. On top of that, if it's not current, > per spec it'll keep an internal reference to the texture (you can avoid the > latter part by making it current) before destroying the texture). Done.
It looks fine overall, but as discussed I think we should split the OutputSurface refactor into another CL so that we can land this more safely
On 2013/02/27 22:47:58, piman wrote: > It looks fine overall, but as discussed I think we should split the > OutputSurface refactor into another CL so that we can land this more safely common changes to messages: https://codereview.chromium.org/12370002/ renderer-side changes: https://codereview.chromium.org/12371002/ browser-side changes: in here
LGTM with one question https://codereview.chromium.org/11861020/diff/36001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/11861020/diff/36001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:1255: if (!frame.gl_frame_data || frame.gl_frame_data->mailbox.isZero()) Do we need to send an ack in the latter case? Or should it just not happen from the renderer pov?
https://codereview.chromium.org/11861020/diff/36001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/11861020/diff/36001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:1255: if (!frame.gl_frame_data || frame.gl_frame_data->mailbox.isZero()) On 2013/02/28 00:10:10, piman wrote: > Do we need to send an ack in the latter case? Or should it just not happen from > the renderer pov? I added it so that this patch on its own would run without spamming errors below with '--enable-compositor-frame-message'. But also I think the renderer should never really send an empty mailbox name, but we can maybe remove it later (or rather add it to the LOG(ERROR) path below). Note that we have to handle it here though because below it actually copies the cc::Mailbox.name into the string (which might be all zeroes, but will never have an empty length). https://codereview.chromium.org/11861020/diff/36001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:1264: ack_callback.Run(true, scoped_refptr<ui::Texture>()); oops, missed a 'return' here
I missed a couple of things initially (noticing now that I'm rebasing my stuff on top of this). https://codereview.chromium.org/11861020/diff/44001/content/port/browser/rend... File content/port/browser/render_widget_host_view_port.h (right): https://codereview.chromium.org/11861020/diff/44001/content/port/browser/rend... content/port/browser/render_widget_host_view_port.h:228: int route_id) {} Actually, this class is pure. You can implement the default, noop in RenderWidgetHostViewBase. Second thing: we shouldn't need to pass render_host_id/route_id : the view can get it from the host (GetRoutingID(), GetProcess()->GetID()), and even then, the view should go back to the host to send the message anyway.
On 2013/02/28 23:16:42, piman wrote: > I missed a couple of things initially (noticing now that I'm rebasing my stuff > on top of this). > > https://codereview.chromium.org/11861020/diff/44001/content/port/browser/rend... > File content/port/browser/render_widget_host_view_port.h (right): > > https://codereview.chromium.org/11861020/diff/44001/content/port/browser/rend... > content/port/browser/render_widget_host_view_port.h:228: int route_id) {} > Actually, this class is pure. You can implement the default, noop in > RenderWidgetHostViewBase. > Second thing: we shouldn't need to pass render_host_id/route_id : the view can > get it from the host (GetRoutingID(), GetProcess()->GetID()), and even then, the > view should go back to the host to send the message anyway. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/11861020/46002
Message was sent while issue was closed.
Change committed as 185468 |
