Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834052:
Avoid flashing when resizing plugins that use PaintManager. Copy the bitmap ... (Closed)
|
Created:
10 years, 5 months ago by jam Modified:
9 years, 7 months ago CC:
chromium-reviews Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAvoid flashing when resizing plugins that use PaintManager. Copy the bitmap from the old backing store to the new one so that we have something to display until the plugin repaints. Credit to Darin for tracking this down and the suggested fix.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52811
Patch Set 1 #
Messages
Total messages: 14 (0 generated)
whoever has time to review this first :)
LGTM
I think we should avoid creating the new device until we paint. This will save the copy and give the same result. On Fri, Jul 16, 2010 at 7:53 PM, <darin@chromium.org> wrote: > LGTM > > http://codereview.chromium.org/2834052/show >
Do you mind if I check this in as an interim measure against the flash? I don't want to enable the internal pdf plugin by default until the flashes are gone. On Fri, Jul 16, 2010 at 9:53 PM, Brett Wilson <brettw@chromium.org> wrote: > I think we should avoid creating the new device until we paint. This > will save the copy and give the same result. > > On Fri, Jul 16, 2010 at 7:53 PM, <darin@chromium.org> wrote: > > LGTM > > > > http://codereview.chromium.org/2834052/show > > >
Hmm... I was thinking about this more too. What happens if the painting and Flush call happen within ViewChanged? Then there should be no flashing, right? Oh, but the problem is that the PaintManager doesn't give us a way to trigger the Flush call directly, because we have to wait for a round trip through the message loop. Hmm... seems like PaintManager should have a method that allows you to paint now. Then, we would not need this CL. -Darin On Fri, Jul 16, 2010 at 10:19 PM, John Abd-El-Malek <jam@chromium.org>wrote: > Do you mind if I check this in as an interim measure against the flash? I > don't want to enable the internal pdf plugin by default until the flashes > are gone. > > > On Fri, Jul 16, 2010 at 9:53 PM, Brett Wilson <brettw@chromium.org> wrote: > >> I think we should avoid creating the new device until we paint. This >> will save the copy and give the same result. >> >> On Fri, Jul 16, 2010 at 7:53 PM, <darin@chromium.org> wrote: >> > LGTM >> > >> > http://codereview.chromium.org/2834052/show >> > >> > >
On Fri, Jul 16, 2010 at 10:22 PM, Darin Fisher <darin@chromium.org> wrote: > Hmm... I was thinking about this more too. What happens if the painting and > Flush call happen within ViewChanged? Then there should be no flashing, > right? > Oh, but the problem is that the PaintManager doesn't give us a way to > trigger the Flush call directly, because we have to wait for a round trip > through the message loop. Hmm... seems like PaintManager should have a > method that allows you to paint now. > Then, we would not need this CL. I originally had that. The problem is that it's not possible to paint now if you already have a flush pending. Rather than have a "paint now that may or may not work" I preferred to have only asynchronous paints. You could have a flush pending even during resize. Now, if you created a new device, we can guarantee that the flush will work, which might cover this resize case. But it would really only work for the built-in plugins anyway since NaCl would run them out of process and resize would be asynchronous. I would prefer to do that only as a last resort for the reader. Brett
On Fri, Jul 16, 2010 at 10:27 PM, Brett Wilson <brettw@chromium.org> wrote: > On Fri, Jul 16, 2010 at 10:22 PM, Darin Fisher <darin@chromium.org> wrote: > > Hmm... I was thinking about this more too. What happens if the painting > and > > Flush call happen within ViewChanged? Then there should be no flashing, > > right? > > Oh, but the problem is that the PaintManager doesn't give us a way to > > trigger the Flush call directly, because we have to wait for a round trip > > through the message loop. Hmm... seems like PaintManager should have a > > method that allows you to paint now. > > Then, we would not need this CL. > > I originally had that. The problem is that it's not possible to paint > now if you already have a flush pending. Rather than have a "paint now > that may or may not work" I preferred to have only asynchronous > paints. You could have a flush pending even during resize. > > Yeah, I see. > Now, if you created a new device, we can guarantee that the flush will > work, which might cover this resize case. But it would really only > work for the built-in plugins anyway since NaCl would run them out of > process and resize would be asynchronous. I would prefer to do that > only as a last resort for the reader. > > Hmm, that's true... unless the IPC between NaCl and renderer is synchronous (the threads are interlocked), which I believe it is. If that were not the case... how would deferring creation of the new DC until we paint work? Wouldn't there still be a race condition? -Darin
On Fri, Jul 16, 2010 at 10:48 PM, Darin Fisher <darin@chromium.org> wrote: > On Fri, Jul 16, 2010 at 10:27 PM, Brett Wilson <brettw@chromium.org> wrote: >> >> On Fri, Jul 16, 2010 at 10:22 PM, Darin Fisher <darin@chromium.org> wrote: >> > Hmm... I was thinking about this more too. What happens if the painting >> > and >> > Flush call happen within ViewChanged? Then there should be no flashing, >> > right? >> > Oh, but the problem is that the PaintManager doesn't give us a way to >> > trigger the Flush call directly, because we have to wait for a round >> > trip >> > through the message loop. Hmm... seems like PaintManager should have a >> > method that allows you to paint now. >> > Then, we would not need this CL. >> >> I originally had that. The problem is that it's not possible to paint >> now if you already have a flush pending. Rather than have a "paint now >> that may or may not work" I preferred to have only asynchronous >> paints. You could have a flush pending even during resize. >> > > Yeah, I see. > >> >> Now, if you created a new device, we can guarantee that the flush will >> work, which might cover this resize case. But it would really only >> work for the built-in plugins anyway since NaCl would run them out of >> process and resize would be asynchronous. I would prefer to do that >> only as a last resort for the reader. >> > > Hmm, that's true... unless the IPC between NaCl and renderer is synchronous > (the > threads are interlocked), which I believe it is. > If that were not the case... how would deferring creation of the new DC > until we > paint work? Wouldn't there still be a race condition? It would be one frame behind in the case of a resize. I don't know whether this is a "race" or not. I guess it means we'll paint twice. I think this is one reason why it's very bad to interlock the NaCl thread. Any time there is a plugin, our hopes of doing synchronous resizing will go down dramatically because we have to wait for a round trip to another process, and for an arbitrary plugin to paint. Brett
On Fri, Jul 16, 2010 at 10:58 PM, Brett Wilson <brettw@chromium.org> wrote: > On Fri, Jul 16, 2010 at 10:48 PM, Darin Fisher <darin@chromium.org> wrote: > > On Fri, Jul 16, 2010 at 10:27 PM, Brett Wilson <brettw@chromium.org> > wrote: > >> > >> On Fri, Jul 16, 2010 at 10:22 PM, Darin Fisher <darin@chromium.org> > wrote: > >> > Hmm... I was thinking about this more too. What happens if the > painting > >> > and > >> > Flush call happen within ViewChanged? Then there should be no > flashing, > >> > right? > >> > Oh, but the problem is that the PaintManager doesn't give us a way to > >> > trigger the Flush call directly, because we have to wait for a round > >> > trip > >> > through the message loop. Hmm... seems like PaintManager should have > a > >> > method that allows you to paint now. > >> > Then, we would not need this CL. > >> > >> I originally had that. The problem is that it's not possible to paint > >> now if you already have a flush pending. Rather than have a "paint now > >> that may or may not work" I preferred to have only asynchronous > >> paints. You could have a flush pending even during resize. > >> > > > > Yeah, I see. > > > >> > >> Now, if you created a new device, we can guarantee that the flush will > >> work, which might cover this resize case. But it would really only > >> work for the built-in plugins anyway since NaCl would run them out of > >> process and resize would be asynchronous. I would prefer to do that > >> only as a last resort for the reader. > >> > > > > Hmm, that's true... unless the IPC between NaCl and renderer is > synchronous > > (the > > threads are interlocked), which I believe it is. > > If that were not the case... how would deferring creation of the new DC > > until we > > paint work? Wouldn't there still be a race condition? > > It would be one frame behind in the case of a resize. I don't know > whether this is a "race" or not. I guess it means we'll paint twice. > > I think this is one reason why it's very bad to interlock the NaCl > thread. Any time there is a plugin, our hopes of doing synchronous > resizing will go down dramatically because we have to wait for a round > trip to another process, and for an arbitrary plugin to paint. > > Brett > Being one frame behind is OK. That's what John's patch here does. The race I was referring to was the interval between BindGraphicsDeviceContext and Flush. Those would have to be processed together before yielding to the message loop, or else we could get a similar flicker problem. WebKit could end up painting from the DC that has been bound but not yet painted. Or, can we paint the DC, and then bind it? (I agree about the concern with interlocking threads!) -Darin
On Fri, Jul 16, 2010 at 11:08 PM, Darin Fisher <darin@chromium.org> wrote: > On Fri, Jul 16, 2010 at 10:58 PM, Brett Wilson <brettw@chromium.org>wrote: > >> On Fri, Jul 16, 2010 at 10:48 PM, Darin Fisher <darin@chromium.org> >> wrote: >> > On Fri, Jul 16, 2010 at 10:27 PM, Brett Wilson <brettw@chromium.org> >> wrote: >> >> >> >> On Fri, Jul 16, 2010 at 10:22 PM, Darin Fisher <darin@chromium.org> >> wrote: >> >> > Hmm... I was thinking about this more too. What happens if the >> painting >> >> > and >> >> > Flush call happen within ViewChanged? Then there should be no >> flashing, >> >> > right? >> >> > Oh, but the problem is that the PaintManager doesn't give us a way to >> >> > trigger the Flush call directly, because we have to wait for a round >> >> > trip >> >> > through the message loop. Hmm... seems like PaintManager should have >> a >> >> > method that allows you to paint now. >> >> > Then, we would not need this CL. >> >> >> >> I originally had that. The problem is that it's not possible to paint >> >> now if you already have a flush pending. Rather than have a "paint now >> >> that may or may not work" I preferred to have only asynchronous >> >> paints. You could have a flush pending even during resize. >> >> >> > >> > Yeah, I see. >> > >> >> >> >> Now, if you created a new device, we can guarantee that the flush will >> >> work, which might cover this resize case. But it would really only >> >> work for the built-in plugins anyway since NaCl would run them out of >> >> process and resize would be asynchronous. I would prefer to do that >> >> only as a last resort for the reader. >> >> >> > >> > Hmm, that's true... unless the IPC between NaCl and renderer is >> synchronous >> > (the >> > threads are interlocked), which I believe it is. >> > If that were not the case... how would deferring creation of the new DC >> > until we >> > paint work? Wouldn't there still be a race condition? >> >> It would be one frame behind in the case of a resize. I don't know >> whether this is a "race" or not. I guess it means we'll paint twice. >> >> I think this is one reason why it's very bad to interlock the NaCl >> thread. Any time there is a plugin, our hopes of doing synchronous >> resizing will go down dramatically because we have to wait for a round >> trip to another process, and for an arbitrary plugin to paint. >> >> Brett >> > > Being one frame behind is OK. > ^^^ correction: OK, but not great. I'm sad that there is no way to eliminate laggy resize even in the case that the plugin is fast to paint. > That's what John's patch here does. > > The race I was referring to was the interval between > BindGraphicsDeviceContext > and Flush. Those would have to be processed together before yielding to > the > message loop, or else we could get a similar flicker problem. WebKit could > end > up painting from the DC that has been bound but not yet painted. > > Or, can we paint the DC, and then bind it? > > (I agree about the concern with interlocking threads!) > > -Darin >
On Fri, Jul 16, 2010 at 11:08 PM, Darin Fisher <darin@chromium.org> wrote: > On Fri, Jul 16, 2010 at 10:58 PM, Brett Wilson <brettw@chromium.org> wrote: >> >> On Fri, Jul 16, 2010 at 10:48 PM, Darin Fisher <darin@chromium.org> wrote: >> > On Fri, Jul 16, 2010 at 10:27 PM, Brett Wilson <brettw@chromium.org> >> > wrote: >> >> >> >> On Fri, Jul 16, 2010 at 10:22 PM, Darin Fisher <darin@chromium.org> >> >> wrote: >> >> > Hmm... I was thinking about this more too. What happens if the >> >> > painting >> >> > and >> >> > Flush call happen within ViewChanged? Then there should be no >> >> > flashing, >> >> > right? >> >> > Oh, but the problem is that the PaintManager doesn't give us a way to >> >> > trigger the Flush call directly, because we have to wait for a round >> >> > trip >> >> > through the message loop. Hmm... seems like PaintManager should have >> >> > a >> >> > method that allows you to paint now. >> >> > Then, we would not need this CL. >> >> >> >> I originally had that. The problem is that it's not possible to paint >> >> now if you already have a flush pending. Rather than have a "paint now >> >> that may or may not work" I preferred to have only asynchronous >> >> paints. You could have a flush pending even during resize. >> >> >> > >> > Yeah, I see. >> > >> >> >> >> Now, if you created a new device, we can guarantee that the flush will >> >> work, which might cover this resize case. But it would really only >> >> work for the built-in plugins anyway since NaCl would run them out of >> >> process and resize would be asynchronous. I would prefer to do that >> >> only as a last resort for the reader. >> >> >> > >> > Hmm, that's true... unless the IPC between NaCl and renderer is >> > synchronous >> > (the >> > threads are interlocked), which I believe it is. >> > If that were not the case... how would deferring creation of the new DC >> > until we >> > paint work? Wouldn't there still be a race condition? >> >> It would be one frame behind in the case of a resize. I don't know >> whether this is a "race" or not. I guess it means we'll paint twice. >> >> I think this is one reason why it's very bad to interlock the NaCl >> thread. Any time there is a plugin, our hopes of doing synchronous >> resizing will go down dramatically because we have to wait for a round >> trip to another process, and for an arbitrary plugin to paint. >> >> Brett > > Being one frame behind is OK. That's what John's patch here does. > The race I was referring to was the interval between > BindGraphicsDeviceContext > and Flush. Those would have to be processed together before yielding to the > message loop, or else we could get a similar flicker problem. WebKit could > end > up painting from the DC that has been bound but not yet painted. > Or, can we paint the DC, and then bind it? It will work to paint it, flush it, and then bind it. The "weird" part is that the Flush callback will be issued just as a PostTask rather than corresponding to anything being painted to the screen. Brett
On Sun, Jul 18, 2010 at 11:17 AM, Brett Wilson <brettw@chromium.org> wrote: > On Fri, Jul 16, 2010 at 11:08 PM, Darin Fisher <darin@chromium.org> wrote: > > On Fri, Jul 16, 2010 at 10:58 PM, Brett Wilson <brettw@chromium.org> > wrote: > >> > >> On Fri, Jul 16, 2010 at 10:48 PM, Darin Fisher <darin@chromium.org> > wrote: > >> > On Fri, Jul 16, 2010 at 10:27 PM, Brett Wilson <brettw@chromium.org> > >> > wrote: > >> >> > >> >> On Fri, Jul 16, 2010 at 10:22 PM, Darin Fisher <darin@chromium.org> > >> >> wrote: > >> >> > Hmm... I was thinking about this more too. What happens if the > >> >> > painting > >> >> > and > >> >> > Flush call happen within ViewChanged? Then there should be no > >> >> > flashing, > >> >> > right? > >> >> > Oh, but the problem is that the PaintManager doesn't give us a way > to > >> >> > trigger the Flush call directly, because we have to wait for a > round > >> >> > trip > >> >> > through the message loop. Hmm... seems like PaintManager should > have > >> >> > a > >> >> > method that allows you to paint now. > >> >> > Then, we would not need this CL. > >> >> > >> >> I originally had that. The problem is that it's not possible to paint > >> >> now if you already have a flush pending. Rather than have a "paint > now > >> >> that may or may not work" I preferred to have only asynchronous > >> >> paints. You could have a flush pending even during resize. > >> >> > >> > > >> > Yeah, I see. > >> > > >> >> > >> >> Now, if you created a new device, we can guarantee that the flush > will > >> >> work, which might cover this resize case. But it would really only > >> >> work for the built-in plugins anyway since NaCl would run them out of > >> >> process and resize would be asynchronous. I would prefer to do that > >> >> only as a last resort for the reader. > >> >> > >> > > >> > Hmm, that's true... unless the IPC between NaCl and renderer is > >> > synchronous > >> > (the > >> > threads are interlocked), which I believe it is. > >> > If that were not the case... how would deferring creation of the new > DC > >> > until we > >> > paint work? Wouldn't there still be a race condition? > >> > >> It would be one frame behind in the case of a resize. I don't know > >> whether this is a "race" or not. I guess it means we'll paint twice. > >> > >> I think this is one reason why it's very bad to interlock the NaCl > >> thread. Any time there is a plugin, our hopes of doing synchronous > >> resizing will go down dramatically because we have to wait for a round > >> trip to another process, and for an arbitrary plugin to paint. > >> > >> Brett > > > > Being one frame behind is OK. That's what John's patch here does. > > The race I was referring to was the interval between > > BindGraphicsDeviceContext > > and Flush. Those would have to be processed together before yielding to > the > > message loop, or else we could get a similar flicker problem. WebKit > could > > end > > up painting from the DC that has been bound but not yet painted. > > Or, can we paint the DC, and then bind it? > > It will work to paint it, flush it, and then bind it. The "weird" part > is that the Flush callback will be issued just as a PostTask rather > than corresponding to anything being painted to the screen. > > Brett > Can we fold this tricky logic into the PaintManager, so that when you resize the PaintManager, it knows to call OnPaint to repaint everything into a new offscreen DC? Our goal should be to have OnPaint called from within ViewChanged so that we can push pixels to the renderer before returning from ViewChanged. That way, the renderer's first paint after a resize will include the newly rendered plugin bits. This means binding the DC before the flush completion has arrived. On the host side, we could notice that the DC becomes bound and then "no-op" the existing PostTask, and defer the flush completion until the browser has painted. This seems workable, though I agree it complicates some of our code. The results should be worth it. If a plugin can paint within 40ms (or whatever our resize paint delay is in the browser these days), then we should get seamless resizing for full page plugins. -Darin
On Sun, Jul 18, 2010 at 8:42 PM, Darin Fisher <darin@chromium.org> wrote: > On Sun, Jul 18, 2010 at 11:17 AM, Brett Wilson <brettw@chromium.org> wrote: >> >> On Fri, Jul 16, 2010 at 11:08 PM, Darin Fisher <darin@chromium.org> wrote: >> > On Fri, Jul 16, 2010 at 10:58 PM, Brett Wilson <brettw@chromium.org> >> > wrote: >> >> >> >> On Fri, Jul 16, 2010 at 10:48 PM, Darin Fisher <darin@chromium.org> >> >> wrote: >> >> > On Fri, Jul 16, 2010 at 10:27 PM, Brett Wilson <brettw@chromium.org> >> >> > wrote: >> >> >> >> >> >> On Fri, Jul 16, 2010 at 10:22 PM, Darin Fisher <darin@chromium.org> >> >> >> wrote: >> >> >> > Hmm... I was thinking about this more too. What happens if the >> >> >> > painting >> >> >> > and >> >> >> > Flush call happen within ViewChanged? Then there should be no >> >> >> > flashing, >> >> >> > right? >> >> >> > Oh, but the problem is that the PaintManager doesn't give us a way >> >> >> > to >> >> >> > trigger the Flush call directly, because we have to wait for a >> >> >> > round >> >> >> > trip >> >> >> > through the message loop. Hmm... seems like PaintManager should >> >> >> > have >> >> >> > a >> >> >> > method that allows you to paint now. >> >> >> > Then, we would not need this CL. >> >> >> >> >> >> I originally had that. The problem is that it's not possible to >> >> >> paint >> >> >> now if you already have a flush pending. Rather than have a "paint >> >> >> now >> >> >> that may or may not work" I preferred to have only asynchronous >> >> >> paints. You could have a flush pending even during resize. >> >> >> >> >> > >> >> > Yeah, I see. >> >> > >> >> >> >> >> >> Now, if you created a new device, we can guarantee that the flush >> >> >> will >> >> >> work, which might cover this resize case. But it would really only >> >> >> work for the built-in plugins anyway since NaCl would run them out >> >> >> of >> >> >> process and resize would be asynchronous. I would prefer to do that >> >> >> only as a last resort for the reader. >> >> >> >> >> > >> >> > Hmm, that's true... unless the IPC between NaCl and renderer is >> >> > synchronous >> >> > (the >> >> > threads are interlocked), which I believe it is. >> >> > If that were not the case... how would deferring creation of the new >> >> > DC >> >> > until we >> >> > paint work? Wouldn't there still be a race condition? >> >> >> >> It would be one frame behind in the case of a resize. I don't know >> >> whether this is a "race" or not. I guess it means we'll paint twice. >> >> >> >> I think this is one reason why it's very bad to interlock the NaCl >> >> thread. Any time there is a plugin, our hopes of doing synchronous >> >> resizing will go down dramatically because we have to wait for a round >> >> trip to another process, and for an arbitrary plugin to paint. >> >> >> >> Brett >> > >> > Being one frame behind is OK. That's what John's patch here does. >> > The race I was referring to was the interval between >> > BindGraphicsDeviceContext >> > and Flush. Those would have to be processed together before yielding to >> > the >> > message loop, or else we could get a similar flicker problem. WebKit >> > could >> > end >> > up painting from the DC that has been bound but not yet painted. >> > Or, can we paint the DC, and then bind it? >> >> It will work to paint it, flush it, and then bind it. The "weird" part >> is that the Flush callback will be issued just as a PostTask rather >> than corresponding to anything being painted to the screen. >> >> Brett > > Can we fold this tricky logic into the PaintManager, so that when you > resize the PaintManager, it knows to call OnPaint to repaint everything > into a new offscreen DC? > Our goal should be to have OnPaint called from within ViewChanged > so that we can push pixels to the renderer before returning from > ViewChanged. That way, the renderer's first paint after a resize will > include the newly rendered plugin bits. > This means binding the DC before the flush completion has arrived. > On the host side, we could notice that the DC becomes bound and > then "no-op" the existing PostTask, and defer the flush completion > until the browser has painted. > This seems workable, though I agree it complicates some of our > code. The results should be worth it. If a plugin can paint within > 40ms (or whatever our resize paint delay is in the browser these > days), then we should get seamless resizing for full page plugins. That sounds good, I copied and pasted your suggestion into the bug. Brett |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
