Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2057143003:
Increase PDF display timeouts (Closed)
|
Created:
4 years, 6 months ago by brucedawson Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIncrease PDF display timeouts
On moderately to very complex PDFs it can take more than 10 ms to
render the page, especially when scrolling. This means that rendering
hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated
without any pixels going to the screen. This means that (other than
scrollbar movement) the user gets *zero* visual feedback until the
mouse/mouse-wheel/key-presses slow down enough to get a fast-frame
in.
This change increases the timeouts. Any PDFs that can render their
scrolled image in less than the timeout will now be continuously
updated.
Because the painting is in a separate process from the renderer
and scrollbars there is little downside to increasing the timeout.
The only downside is a slight increase in the latency of rendering
the final frame when the user stops scrolling a complex PDF.
For complex/expensive PDFs the typical increase in latency will
be half of the floor of the PDF render time and the timeout,
so about 125 ms for complex/expensive PDFs.
For trivial PDFs the increased timeout will make no difference.
On the test pages from the four attached bugs the improvement is
dramatic. Scrolling is sometimes slow, but frames continue being
delivered without the user having to pause and wait for Chrome to
catch up.
Improving the performance of decoding and scaling images, or doing
the decoding and/or scaling asynchronously while still displaying
scrolling PDF imagery, is left for a future bug. This fix is
necessary for future improvements but does not make other
improvements unnecessary.
BUG=318175, 582752, 617365, 619117
Committed: https://crrev.com/60e37416d6640d90cc6942b361d36cb77531174f
Cr-Commit-Position: refs/heads/master@{#401660}
Patch Set 1 #Patch Set 2 : Remove all Pause_NeedToPauseNow code #Patch Set 3 : Set timeouts to 200 and 100 #Patch Set 4 : Tweaking comment and values #Messages
Total messages: 22 (8 generated)
Description was changed from ========== Increase progressive-paint timeouts to avoid zero fps On moderately to very complex PDFs it can take more than 10 ms to render the page, especially when scrolling. This means that rendering hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated without any pixels going to the screen. This means that (other than scrollbar movement) the user gets *zero* visual feedback until the mouse/mouse-wheel/key-presses slow down enough to get a fast-frame in. This change simply increases the timeouts so that frames are rendered as quickly as possible. BUG=619117 ========== to ========== Disable PDF display timeouts On moderately to very complex PDFs it can take more than 10 ms to render the page, especially when scrolling. This means that rendering hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated without any pixels going to the screen. This means that (other than scrollbar movement) the user gets *zero* visual feedback until the mouse/mouse-wheel/key-presses slow down enough to get a fast-frame in. This change removes the timeouts. This makes the scrollbar less responsive, but makes the PDF itself much more responsive. On the test pages from the four attached bugs the improvement is dramatic. Scrolling is sometimes slow, but frames continue being delivered without the user having to pause and wait for Chrome to catch up. Improving the performance of decoding and scaling images, or doing the decoding and/or scaling asynchronously while still displaying scrolling PDF imagery, is left for a future bug. This fix is necessary for future improvements but does not make other improvements unnecessary. BUG=318175,582752,617365,619117 ==========
brucedawson@chromium.org changed reviewers: + jam@chromium.org
PTAL? Improving the image decoding is a follow-up task that I'm working on, but getting rid of this timeout is important. I have been unable to find a scenario where the timeout makes things better. Please let me know if I'm wrong.
jam@chromium.org changed reviewers: + thestig@chromium.org
On 2016/06/18 00:34:40, brucedawson wrote: > PTAL? > > Improving the image decoding is a follow-up task that I'm working on, but > getting rid of this timeout is important. > > I have been unable to find a scenario where the timeout makes things better. > Please let me know if I'm wrong. It's a tradeoff. There's the use case where you describe, where we might not show anything till the user stops scrolling. We decided to make this worse for the other use case below. The reason this logic got added was for really slow PDFs that could take a few seconds to render. https://www.sfmta.com/sites/default/files/pressreleases/2015/Muni%20Map%20201... is one example if I remember correctly. In that case, we were seeing the renderer hang for multiple seconds. An argument can be made now that at least we're not hanging the renderer for multiple seconds since PDF is in a different process. This still leaves the PDF plugin process possibly hung, but at least if the renderer process has other tabs they're not hung. Perhaps we can be smarter by for example using dynamic thresholds, increasing it if after n scrolls we never get data because the (increasing) timeout is still less than how long it takes to render. The extreme would be removing it altogether like this. Note I'm not working in this area anymore, so adding Lei for his thoughts as well.
Thanks for the example PDF. I'll add that to my list of torture tests. The timeout would be much more valuable if the partial results were actually displayed, or if the bitmap was at least scrolled. Displaying nothing at all (potentially for many seconds at a time, if you keep scrolling) is frustrating. I can see that there is some value in updating the scrollbar at a high frequency, but I'm just not convinced that the scrollbar animation is important enough to justify not updating the image bitmap at all. I admit I'm torn. Part of me wants to increase the timeout to 100-200 ms so that most PDFs can scroll in the time available most of the time. Part of me thinks that even that is silly because then there will be some PDFs that still can't display in that amount of time, and now the scrollbar frame rate will be 10 fps instead of 60 fps - worse on both axes. That's the logic that led me to disable the feature completely. One other viewer I looked at seemed to also have a timeout but if that timeout was reached then it would scroll the bitmap, so there was always an illusion of movement. That might be a better long-term fix. So... if we're not sure that disabling the timeout entirely is the best solution then I think we should increase it - baby steps. Multiply both timeouts by a factor of ten? BTW, I have a fix for one of the image decode functions which will double its performance and help at least some PDFs. Decode time should drop from 300 ms to 150 ms.
On 2016/06/20 17:38:23, brucedawson wrote: > Thanks for the example PDF. I'll add that to my list of torture tests. > > The timeout would be much more valuable if the partial results were actually > displayed, or if the bitmap was at least scrolled. Displaying nothing at all > (potentially for many seconds at a time, if you keep scrolling) is frustrating. > I can see that there is some value in updating the scrollbar at a high > frequency, but I'm just not convinced that the scrollbar animation is important > enough to justify not updating the image bitmap at all. Agree on the above. AFAIK this would all be fixed if https://bugs.chromium.org/p/chromium/issues/detail?id=439283#c3 is fixed. > > I admit I'm torn. Part of me wants to increase the timeout to 100-200 ms so that > most PDFs can scroll in the time available most of the time. > > Part of me thinks that even that is silly because then there will be some PDFs > that still can't display in that amount of time, and now the scrollbar frame > rate will be 10 fps instead of 60 fps - worse on both axes. That's the logic > that led me to disable the feature completely. > > One other viewer I looked at seemed to also have a timeout but if that timeout > was reached then it would scroll the bitmap, so there was always an illusion of > movement. That might be a better long-term fix. > > > So... if we're not sure that disabling the timeout entirely is the best solution > then I think we should increase it - baby steps. Multiply both timeouts by a > factor of ten? It's hard to know the effects of this. IMO we should put all efforts towards https://bugs.chromium.org/p/chromium/issues/detail?id=439283#c3 which is the proper fix. > > BTW, I have a fix for one of the image decode functions which will double its > performance and help at least some PDFs. Decode time should drop from 300 ms to > 150 ms.
Description was changed from ========== Disable PDF display timeouts On moderately to very complex PDFs it can take more than 10 ms to render the page, especially when scrolling. This means that rendering hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated without any pixels going to the screen. This means that (other than scrollbar movement) the user gets *zero* visual feedback until the mouse/mouse-wheel/key-presses slow down enough to get a fast-frame in. This change removes the timeouts. This makes the scrollbar less responsive, but makes the PDF itself much more responsive. On the test pages from the four attached bugs the improvement is dramatic. Scrolling is sometimes slow, but frames continue being delivered without the user having to pause and wait for Chrome to catch up. Improving the performance of decoding and scaling images, or doing the decoding and/or scaling asynchronously while still displaying scrolling PDF imagery, is left for a future bug. This fix is necessary for future improvements but does not make other improvements unnecessary. BUG=318175,582752,617365,619117 ========== to ========== Increase PDF display timeouts On moderately to very complex PDFs it can take more than 10 ms to render the page, especially when scrolling. This means that rendering hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated without any pixels going to the screen. This means that (other than scrollbar movement) the user gets *zero* visual feedback until the mouse/mouse-wheel/key-presses slow down enough to get a fast-frame in. This change increases the timeouts. This makes the scrollbar less responsive, but makes the PDF itself much more responsive. Because kMaxInitialProgressivePaintTimeMs is set to 100 ms the scrollbars will still be updated at least ~10 times per second, so the worst case is not made much worse, but the best case (smooth scrolling) is far more common. On the test pages from the four attached bugs the improvement is dramatic. Scrolling is sometimes slow, but frames continue being delivered without the user having to pause and wait for Chrome to catch up. Improving the performance of decoding and scaling images, or doing the decoding and/or scaling asynchronously while still displaying scrolling PDF imagery, is left for a future bug. This fix is necessary for future improvements but does not make other improvements unnecessary. Future improvements to use the PPB_Compositor will further improve scrolling and may render this change unnecessary. BUG=318175,582752,617365,619117 ==========
Description was changed from ========== Increase PDF display timeouts On moderately to very complex PDFs it can take more than 10 ms to render the page, especially when scrolling. This means that rendering hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated without any pixels going to the screen. This means that (other than scrollbar movement) the user gets *zero* visual feedback until the mouse/mouse-wheel/key-presses slow down enough to get a fast-frame in. This change increases the timeouts. This makes the scrollbar less responsive, but makes the PDF itself much more responsive. Because kMaxInitialProgressivePaintTimeMs is set to 100 ms the scrollbars will still be updated at least ~10 times per second, so the worst case is not made much worse, but the best case (smooth scrolling) is far more common. On the test pages from the four attached bugs the improvement is dramatic. Scrolling is sometimes slow, but frames continue being delivered without the user having to pause and wait for Chrome to catch up. Improving the performance of decoding and scaling images, or doing the decoding and/or scaling asynchronously while still displaying scrolling PDF imagery, is left for a future bug. This fix is necessary for future improvements but does not make other improvements unnecessary. Future improvements to use the PPB_Compositor will further improve scrolling and may render this change unnecessary. BUG=318175,582752,617365,619117 ========== to ========== Increase PDF display timeouts On moderately to very complex PDFs it can take more than 10 ms to render the page, especially when scrolling. This means that rendering hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated without any pixels going to the screen. This means that (other than scrollbar movement) the user gets *zero* visual feedback until the mouse/mouse-wheel/key-presses slow down enough to get a fast-frame in. This change increases the timeouts. Any PDFs that can render their scrolled image in less than the timeout will now be continuously updated. Because the painting is in a separate process from the renderer and scrollbars there is little downside to increasing the timeout. The only downside is a slight increase in the latency of rendering the final frame when the user stops scrolling a complex PDF. For complex/expensive PDFs the typical increase in latency will be half of the floor of the PDF render time and the timeout, so about 125 ms for complex/expensive PDFs. For trivial PDFs the increased timeout will make no difference. On the test pages from the four attached bugs the improvement is dramatic. Scrolling is sometimes slow, but frames continue being delivered without the user having to pause and wait for Chrome to catch up. Improving the performance of decoding and scaling images, or doing the decoding and/or scaling asynchronously while still displaying scrolling PDF imagery, is left for a future bug. This fix is necessary for future improvements but does not make other improvements unnecessary. BUG=318175,582752,617365,619117 ==========
lgtm per offline thread since 1) the renderer isn't hung since this is occurring in a separate process 2) the scrollbars are still responsive since they're painted by the renderer
btw I don't think all the referenced bugs are fixed by this change, so IMO you should remove all other than the most recent one (619117). the other bugs would be fixed (or at least significantly improved) by using the pepper compositor API per https://bugs.chromium.org/p/pdfium/issues/detail?id=523
On 2016/06/22 22:55:45, jam wrote: > btw I don't think all the referenced bugs are fixed by this change, (to be clear: this is based on my testing on a VM where I don't see a difference before/after on a VM with 4 cores) > so IMO you > should remove all other than the most recent one (619117). the other bugs would > be fixed (or at least significantly improved) by using the pepper compositor API > per https://bugs.chromium.org/p/pdfium/issues/detail?id=523
On 2016/06/22 23:00:58, jam wrote: > On 2016/06/22 22:55:45, jam wrote: > > btw I don't think all the referenced bugs are fixed by this change, My understanding is that it is appropriate to link "related" bugs from a CL, even if they aren't fixed by it. My plan would be to leave most of them open, but having them linked to this CL will make it clear why the results are improved now.
On 2016/06/22 23:32:50, brucedawson wrote: > On 2016/06/22 23:00:58, jam wrote: > > On 2016/06/22 22:55:45, jam wrote: > > > btw I don't think all the referenced bugs are fixed by this change, > > My understanding is that it is appropriate to link "related" bugs from a CL, > even if they aren't fixed by it. > > My plan would be to leave most of them open, but having them linked to this CL > will make it clear why the results are improved now. (per comment 12) I couldn't repro an improvement in a 4 core VM for the other bugs. It looks like Raymes couldn't see an improvement either. I guess my question is: how can we repro this?
> (per comment 12) I couldn't repro an improvement in a 4 core VM for the other > bugs. It looks like Raymes couldn't see an improvement either. > I guess my question is: how can we repro this? Huh. That's curious. I can see enormous improvements on the sample PDFs from all of the bugs. For all of the sample PDFs there is a fairly wide scroll range where my change makes the difference between zero updates, and a pretty decent scrolling speed. I'm trying this on a real machine - maybe the VM's display stack somehow negates the benefit? Or maybe the mouse move messages in the VM are throttled enough that the problem does not manifest - if the mouse move messages were throttled to ~20 per second then many of the PDFs would probably keep up fine. Taking http://www.valvesoftware.com/publications/2011/ValveXperf-Dawson.pdf as an example, if I scroll down fairly slowly (taking 10-15 seconds to go the length of the document) then I get zero fps on stable, and ~20 fps on my local build. No matter what speed I scroll I can't get any frames generated when the document fills half of my monitor. If I shrink it drastically I occasionally get frames. If you are using a debug build your results will be worse of course. If you use my test build without specifying 250,300 then your results will not be improved as much as the final versions. Do you see smooth scrolling in all cases, or in none?
> Do you see smooth scrolling in all cases, or in none? I did further testing on a cheap consumer laptop and was able to see significant improvements. I'm not sure why jam is getting poor scrolling with and without the fix. It may be that he is still hitting the timeout, or there may be some additional issue that my testing is somehow avoiding. Anyway, this improves some scenarios dramatically (all of them for me) with no significant downside. Committing.
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2057143003/60001
Message was sent while issue was closed.
Description was changed from ========== Increase PDF display timeouts On moderately to very complex PDFs it can take more than 10 ms to render the page, especially when scrolling. This means that rendering hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated without any pixels going to the screen. This means that (other than scrollbar movement) the user gets *zero* visual feedback until the mouse/mouse-wheel/key-presses slow down enough to get a fast-frame in. This change increases the timeouts. Any PDFs that can render their scrolled image in less than the timeout will now be continuously updated. Because the painting is in a separate process from the renderer and scrollbars there is little downside to increasing the timeout. The only downside is a slight increase in the latency of rendering the final frame when the user stops scrolling a complex PDF. For complex/expensive PDFs the typical increase in latency will be half of the floor of the PDF render time and the timeout, so about 125 ms for complex/expensive PDFs. For trivial PDFs the increased timeout will make no difference. On the test pages from the four attached bugs the improvement is dramatic. Scrolling is sometimes slow, but frames continue being delivered without the user having to pause and wait for Chrome to catch up. Improving the performance of decoding and scaling images, or doing the decoding and/or scaling asynchronously while still displaying scrolling PDF imagery, is left for a future bug. This fix is necessary for future improvements but does not make other improvements unnecessary. BUG=318175,582752,617365,619117 ========== to ========== Increase PDF display timeouts On moderately to very complex PDFs it can take more than 10 ms to render the page, especially when scrolling. This means that rendering hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated without any pixels going to the screen. This means that (other than scrollbar movement) the user gets *zero* visual feedback until the mouse/mouse-wheel/key-presses slow down enough to get a fast-frame in. This change increases the timeouts. Any PDFs that can render their scrolled image in less than the timeout will now be continuously updated. Because the painting is in a separate process from the renderer and scrollbars there is little downside to increasing the timeout. The only downside is a slight increase in the latency of rendering the final frame when the user stops scrolling a complex PDF. For complex/expensive PDFs the typical increase in latency will be half of the floor of the PDF render time and the timeout, so about 125 ms for complex/expensive PDFs. For trivial PDFs the increased timeout will make no difference. On the test pages from the four attached bugs the improvement is dramatic. Scrolling is sometimes slow, but frames continue being delivered without the user having to pause and wait for Chrome to catch up. Improving the performance of decoding and scaling images, or doing the decoding and/or scaling asynchronously while still displaying scrolling PDF imagery, is left for a future bug. This fix is necessary for future improvements but does not make other improvements unnecessary. BUG=318175,582752,617365,619117 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Increase PDF display timeouts On moderately to very complex PDFs it can take more than 10 ms to render the page, especially when scrolling. This means that rendering hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated without any pixels going to the screen. This means that (other than scrollbar movement) the user gets *zero* visual feedback until the mouse/mouse-wheel/key-presses slow down enough to get a fast-frame in. This change increases the timeouts. Any PDFs that can render their scrolled image in less than the timeout will now be continuously updated. Because the painting is in a separate process from the renderer and scrollbars there is little downside to increasing the timeout. The only downside is a slight increase in the latency of rendering the final frame when the user stops scrolling a complex PDF. For complex/expensive PDFs the typical increase in latency will be half of the floor of the PDF render time and the timeout, so about 125 ms for complex/expensive PDFs. For trivial PDFs the increased timeout will make no difference. On the test pages from the four attached bugs the improvement is dramatic. Scrolling is sometimes slow, but frames continue being delivered without the user having to pause and wait for Chrome to catch up. Improving the performance of decoding and scaling images, or doing the decoding and/or scaling asynchronously while still displaying scrolling PDF imagery, is left for a future bug. This fix is necessary for future improvements but does not make other improvements unnecessary. BUG=318175,582752,617365,619117 ========== to ========== Increase PDF display timeouts On moderately to very complex PDFs it can take more than 10 ms to render the page, especially when scrolling. This means that rendering hits the kMaxInitialProgressivePaintTimeMs timeout and is terminated without any pixels going to the screen. This means that (other than scrollbar movement) the user gets *zero* visual feedback until the mouse/mouse-wheel/key-presses slow down enough to get a fast-frame in. This change increases the timeouts. Any PDFs that can render their scrolled image in less than the timeout will now be continuously updated. Because the painting is in a separate process from the renderer and scrollbars there is little downside to increasing the timeout. The only downside is a slight increase in the latency of rendering the final frame when the user stops scrolling a complex PDF. For complex/expensive PDFs the typical increase in latency will be half of the floor of the PDF render time and the timeout, so about 125 ms for complex/expensive PDFs. For trivial PDFs the increased timeout will make no difference. On the test pages from the four attached bugs the improvement is dramatic. Scrolling is sometimes slow, but frames continue being delivered without the user having to pause and wait for Chrome to catch up. Improving the performance of decoding and scaling images, or doing the decoding and/or scaling asynchronously while still displaying scrolling PDF imagery, is left for a future bug. This fix is necessary for future improvements but does not make other improvements unnecessary. BUG=318175,582752,617365,619117 Committed: https://crrev.com/60e37416d6640d90cc6942b361d36cb77531174f Cr-Commit-Position: refs/heads/master@{#401660} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/60e37416d6640d90cc6942b361d36cb77531174f Cr-Commit-Position: refs/heads/master@{#401660} |
