Closed Bug 732917 Opened 8 years ago Closed 8 years ago

MAPLE: Texture upload causes significant pauses during panning/flinging

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
firefox13 --- affected
firefox14 --- fixed
blocking-fennec1.0 --- beta+

People

(Reporter: cwiiis, Assigned: cwiiis)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx][QA+])

Attachments

(6 files, 17 obsolete files)

937 bytes, patch
cwiiis
: review+
Details | Diff | Splinter Review
7.16 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
4.32 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
3.14 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
17.17 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
6.46 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
When panning, texture upload causes us to pause for significant amounts of time, resulting in 'jank' (jerky-looking updates/unresponsive feedback).

This will be fixed in bug 730718, but it seems that that won't be possible on some devices (well, Adreno devices with crappy drivers (a lot of devices)).

We could mitigate this by uploading tiles asynchronously.

Some WIP patches incoming.
This patch adds push/pop functions to TiledTextureImage, allowing for recursive tile iteration.
This patch adds an optional callback on TiledTextureImage that gets called on tile iteration (practically, when NextTile is called).
This patch just makes TiledTextureImage::DirectUpdate use BeginIteration/NextTile/etc. rather than accessing the internal variables. This causes the tile iteration callback to be called when DirectUpdate is happening.
Attachment #602846 - Attachment is patch: true
Here's the horrible hacky part - This uses the tile iteration callback to time how long we spend uploading tiles and if it reaches over a certain time, spins the compositor message loop.

This allows the app to stay responsive (which works), but doesn't really work correctly and causes updates to block on scrolling too much and incorrect data to show (which is lame).


We did something quite similar with the Java compositor, but the Java compositor had context to know which parts of the layer were important to upload synchronously, and it was tied into screen updates.

I think ideally, we want a mechanism to control the upload via the CompositorParent, rather than this way round.

I think you could go about this by adding a 'DoUploadWork' style function to layers, then either;

1- Double-buffer all layers and return immediately on upload transactions, asynchronously spreading the work of updating tiles later - We'd have to block when a second update comes in and the first isn't finished, but we can throttle updates in the situation that upload can't keep up.

2- Add an async-return method to IPDL so that we can spread the work over time and have rendering block on upload entirely, and return once it's done.

3- ?

Idea 1 kinda sucks for memory use, and would cause extra drawing due to the double-buffer too, idea 2 has the downfall of causing rendering to block on upload for longer than idea 1, but it already does, currently.

On top of this, we probably need that same mechanism to know what part of the layer intersects the screen so that we can update that part of the image synchronously. This should be eas(y/ier) if the upload is controlled from CompositorParent.


Feedback on patches and ideas are gratefully accepted!
Ccing people I think may be interested.
(In reply to Chris Lord [:cwiiis] from comment #4)

> I think you could go about this by adding a 'DoUploadWork' style function to
> layers, then either;
> 
> 1- Double-buffer all layers and return immediately on upload transactions,
> asynchronously spreading the work of updating tiles later - We'd have to
> block when a second update comes in and the first isn't finished, but we can
> throttle updates in the situation that upload can't keep up.
> 
> 2- Add an async-return method to IPDL so that we can spread the work over
> time and have rendering block on upload entirely, and return once it's done.
> 
> 3- ?

Nice work! I like option (1): With our current displayport size and a moderately-sized screen (as we'd find on these lower-end devices), the extra memory would amount to roughly 3 MB, which seems worth it for the benefits we'd get. We could release the extra memory when Fennec goes into the background, to ensure that double-buffering doesn't increase the chances of Fennec getting killed by the system. Also, this option seems to be cleaner and more straightforward to implement.
Ok, I'm going to have a go at implementing option 1.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Comment on attachment 602844 [details] [diff] [review]
Part 1 - Add push/pop for tile iteration

Review of attachment 602844 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContext.h
@@ +445,5 @@
>      virtual PRUint32 GetTileCount();
>      virtual void BeginTileIteration();
>      virtual bool NextTile();
> +    virtual int PushIterator();
> +    virtual bool PopIterator();

I think we need either a better name for them or a comment or what these are for since we don't have a stack of iterators.
Comment on attachment 602847 [details] [diff] [review]
Part X - Spin compositor message loop while tile upload is happening

Review of attachment 602847 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +919,5 @@
> +    if ((now - (*then)) / 1000L > 10) {
> +        printf_stderr("XXXG: Spinning main loop\n");
> +
> +        aImage->PushIterator();
> +        AndroidBridge::Bridge()->RunAllPendingCompositionEvents();

Here you're using the AndroidBridge for it's global variable, we should find a better way to pass the reference of the current compositor (we get one per window) and we could have this code ready to go on desktop. It makes the code cleaner.
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → beta+
Attachment #602847 - Attachment description: Part 4 - Spin compositor message loop while tile upload is happening → Part X - Spin compositor message loop while tile upload is happening
Attachment #602847 - Attachment is obsolete: true
ShadowLayers::ShouldDoubleBuffer has been unused since bug 690469.
This enables double-buffering on the content side for ShadowThebesLayerGL on Android/Adreno, as a build-up to progressive texture upload.
My part 6 enables delayed texture upload, but it's blocking on not having any facility to find out the visibility rect of a Layer with respect to the viewport.

I need to know how much of a layer is visible, but GetVisibleRect doesn't seem to provide such a function.

If we know the 2d rectangle of the layer post-transform (I'm assuming only a 2d transforms here) and we know the screen-size, this should be easily possible. If anyone from gfx would like to chime in with the function that I'm missing that does this, or the 2 lines of code necessary, that'd be great (though I'll be continuing to work on it regardless)
While I still appreciate help here, I think I've cracked how to get the intersecting rectangle. Will hopefully finish up this part tomorrow.
Timothy or Mats might be able to help with your questions in comment 12.
This patch stops Gecko from blocking on texture upload and uploads to the texture on-demand when rendering layers.

Hopefully, just adding the job to upload texture parts progressively is next.

Also need to see about expanding upload regions to tile boundaries, this is currently very inefficient.
Blocks: 734164
Depends on: 734175
Uploading the final part for reference, but the whole lot needs a bit of re-working now.

This results in quite a bit of visible corruption, as bug 734175 was causing me to think I had correct results when I didn't.

On a plus-side, the performance benefit is very much there, so this is definitely worth persevering with :)

This patch incorporates a different fix for the issue in bug 734175, but I'll be rebasing to work on the fix I included there.
Attachment #602844 - Attachment is obsolete: true
Attachment #604626 - Flags: review?(bjacob)
Attachment #602845 - Attachment is obsolete: true
Attachment #604627 - Flags: review?(bjacob)
Attachment #602846 - Attachment is obsolete: true
Attachment #604628 - Flags: review?(bjacob)
Attachment #603762 - Attachment is obsolete: true
Attachment #604629 - Flags: review?(jones.chris.g)
Attachment #603763 - Attachment is obsolete: true
Attachment #604630 - Flags: review?(ajuma)
I'm not sure that the code that figures out what part of the layer intersects with the screen is entirely correct... It seems to work for the most part, but I also seem to *very occasionally* see stale content.

Amazingly, just up to this patch is quite nice on Adreno, seems texture upload is very fast there (but the next part makes it even better).
Attachment #604018 - Attachment is obsolete: true
Attachment #604631 - Flags: review?(matt.woodrow)
Attachment #604631 - Flags: review?(bgirard)
This is pretty much good to go, but calling into AndroidBridge for the compositor thread is obviously a hack.

I think adding an accessor for the compositor parent/thread on the widget would make sense, but that means it'd have to be on nsIWidget, and I'm not sure if we want that or not.

Need feedback/suggestions.
Attachment #604124 - Attachment is obsolete: true
Attachment #604632 - Flags: feedback?(bgirard)
Comment on attachment 604630 [details] [diff] [review]
Part 5 - Enable double-buffering on adreno/android

Review of attachment 604630 [details] [diff] [review]:
-----------------------------------------------------------------

General observation: We should re-factor the double-buffering code to reduce the amount of code duplication this is going to add. However, I'm happy with filing a follow-up bug for the re-factoring (and adding a FIXME comment to ThebesLayerOGL.h).

I'm looking forward to the performance wins these patches should give us!

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +1030,5 @@
> +    if (!mDestroyed) {
> +      if (!mBuffer) {
> +        mBuffer = new ShadowBufferOGL(this);
> +      }
> +      // XXX Is this uploading too much?

Could you elaborate a bit on why this might be uploading too much?

@@ +1062,5 @@
>  {
> +  if (ShouldDoubleBuffer()) {
> +    mFrontBuffer.Clear();
> +    mOldValidRegion.SetEmpty();
> +

Should we also be setting mValidRegion to empty here? If not, a comment about this would be helpful.
Attachment #604630 - Flags: review?(ajuma) → review+
Comment on attachment 604631 [details] [diff] [review]
Part 6 - Upload to texture on demand when double-buffering

Review of attachment 604631 [details] [diff] [review]:
-----------------------------------------------------------------

Matt can give a better look over for the layer transform calculations.

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +961,5 @@
>    NS_ASSERTION(((destBounds.x % size.width) + destBounds.width <= size.width) &&
>                 ((destBounds.y % size.height) + destBounds.height <= size.height),
>                 "Updated region lies across rotation boundaries!");
>  
>    // NB: this gfxContext must not escape EndUpdate() below

This comment got moved incorrectly while refactoring.

@@ +964,5 @@
>  
>    // NB: this gfxContext must not escape EndUpdate() below
> +  if (aDelayUpload) {
> +    // Record the region that needs to be updated
> +    aUploadRegion.Or(aUploadRegion, destRegion).And(aUploadRegion, nsIntRect(0, 0, size.width, size.height));

Let's rename this to aPendingUploadRegion or something more descriptive.

@@ +1033,5 @@
> +  // Do this in possibly 4 chunks, to account for rotation boundaries
> +  nsIntRegion updateRegion;
> +  for (int i = 0; i < 4; i++) {
> +      switch(i) {
> +        case 1:

It's confusing to see 'case 0' ommitted. I think we should add it here with a comment saying it's not needed. Or move 'aRegion.MoveBy(mFrontBuffer.Rotation());' into case 0.

@@ +1114,5 @@
>          mBuffer = new ShadowBufferOGL(this);
>        }
>        // XXX Is this uploading too much?
>        nsRefPtr<gfxASurface> surf = ShadowLayerForwarder::OpenDescriptor(mFrontBufferDescriptor);
> +      mBuffer->Upload(surf, aUpdatedRegion, aNewFront.rect(), aNewFront.rotation(), true, mRegionToUpdate);

Likewise let's rename mRegionToUpdate to imply that it can be pending.
Attachment #604631 - Flags: review?(bgirard) → review+
(In reply to Ali Juma [:ajuma] from comment #24)
> Comment on attachment 604630 [details] [diff] [review]
> Part 5 - Enable double-buffering on adreno/android
> 
> Review of attachment 604630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General observation: We should re-factor the double-buffering code to reduce
> the amount of code duplication this is going to add. However, I'm happy with
> filing a follow-up bug for the re-factoring (and adding a FIXME comment to
> ThebesLayerOGL.h).
> 
> I'm looking forward to the performance wins these patches should give us!
> 
> ::: gfx/layers/opengl/ThebesLayerOGL.cpp
> @@ +1030,5 @@
> > +    if (!mDestroyed) {
> > +      if (!mBuffer) {
> > +        mBuffer = new ShadowBufferOGL(this);
> > +      }
> > +      // XXX Is this uploading too much?
> 
> Could you elaborate a bit on why this might be uploading too much?

I wasn't sure that because we're double-buffering, the updated region will contain some area that the last frame contained and wouldn't actually need updating, because we upload it to a single buffer... But I'm not sure if this is the case, or if it is, if there's any mechanism to work around this safely anyway.

I can add a comment to this effect here.

> @@ +1062,5 @@
> >  {
> > +  if (ShouldDoubleBuffer()) {
> > +    mFrontBuffer.Clear();
> > +    mOldValidRegion.SetEmpty();
> > +
> 
> Should we also be setting mValidRegion to empty here? If not, a comment
> about this would be helpful.

This code was just copied from basic layers, but I guess the reasoning is that because the valid region is replaced every time you swap buffers, and the front buffer doesn't exist after this is called, there's no need to clear its valid region here. (note that the function never did this in the first place)
(In reply to Benoit Girard (:BenWa) from comment #25)
> Comment on attachment 604631 [details] [diff] [review]
> Part 6 - Upload to texture on demand when double-buffering
> 
> Review of attachment 604631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Matt can give a better look over for the layer transform calculations.
> 
> ::: gfx/layers/opengl/ThebesLayerOGL.cpp
> @@ +961,5 @@
> >    NS_ASSERTION(((destBounds.x % size.width) + destBounds.width <= size.width) &&
> >                 ((destBounds.y % size.height) + destBounds.height <= size.height),
> >                 "Updated region lies across rotation boundaries!");
> >  
> >    // NB: this gfxContext must not escape EndUpdate() below
> 
> This comment got moved incorrectly while refactoring.
> 
> @@ +964,5 @@
> >  
> >    // NB: this gfxContext must not escape EndUpdate() below
> > +  if (aDelayUpload) {
> > +    // Record the region that needs to be updated
> > +    aUploadRegion.Or(aUploadRegion, destRegion).And(aUploadRegion, nsIntRect(0, 0, size.width, size.height));
> 
> Let's rename this to aPendingUploadRegion or something more descriptive.
> 
> @@ +1033,5 @@
> > +  // Do this in possibly 4 chunks, to account for rotation boundaries
> > +  nsIntRegion updateRegion;
> > +  for (int i = 0; i < 4; i++) {
> > +      switch(i) {
> > +        case 1:
> 
> It's confusing to see 'case 0' ommitted. I think we should add it here with
> a comment saying it's not needed. Or move
> 'aRegion.MoveBy(mFrontBuffer.Rotation());' into case 0.
> 
> @@ +1114,5 @@
> >          mBuffer = new ShadowBufferOGL(this);
> >        }
> >        // XXX Is this uploading too much?
> >        nsRefPtr<gfxASurface> surf = ShadowLayerForwarder::OpenDescriptor(mFrontBufferDescriptor);
> > +      mBuffer->Upload(surf, aUpdatedRegion, aNewFront.rect(), aNewFront.rotation(), true, mRegionToUpdate);
> 
> Likewise let's rename mRegionToUpdate to imply that it can be pending.

Thanks, will address all of these.
Just a thought, do we just want to enable this on android full-stop for now? Seeing as we don't yet have a threads/gralloc path for texture upload, this (theoretically) ought to benefit all android devices.
Priority: -- → P1
Whiteboard: [gfx]
Comment on attachment 604632 [details] [diff] [review]
Part 7 - Upload progressively, asynchronously

I think what you want is to send a real IPDL message.
Attachment #604632 - Flags: feedback?(bgirard) → feedback-
Comment on attachment 604629 [details] [diff] [review]
Part 4 - Remove ShadowLayers::ShouldDoubleBuffer

># HG changeset patch
># Parent 6f9022834aa261f0131787458f11bdf330be28cb
># User Chris Lord <chrislord.net@gmail.com>
>Bug 732917 - Remove ShadowLayers::ShouldDoubleBuffer. r=
>
>This function has been unused since bug #690469.
>
>diff --git a/gfx/layers/ipc/ShadowLayers.h b/gfx/layers/ipc/ShadowLayers.h
>--- a/gfx/layers/ipc/ShadowLayers.h
>+++ b/gfx/layers/ipc/ShadowLayers.h
>@@ -40,16 +40,17 @@
> 
> #ifndef mozilla_layers_ShadowLayers_h
> #define mozilla_layers_ShadowLayers_h 1
> 
> #include "gfxASurface.h"
> 
> #include "ImageLayers.h"
> #include "Layers.h"
>+#include "GLContext.h"
> 

This shouldn't be here, please remove.

r=me with that
Attachment #604629 - Flags: review?(jones.chris.g) → review+
Addressed comment, carrying over r=cjones.
Attachment #604629 - Attachment is obsolete: true
Attachment #605733 - Flags: review+
Changed to enable it on Android in general, confirmed on IRC, carrying over r=ajuma.

With regards to comments, I didn't add mVisibleRegion.SetEmpty, as this wasn't there before (so if it was a problem, it was a pre-existing one and I think should be handled in a separate bug - but I think it's ok).

I've removed the comment about uploading too much - there's really nothing we can do there beyond adding some mechanism to tell Gecko when the upload's finished and it can write to the front-buffer.

It'd be nice to do this, but I can't imagine it'd have a large impact - this can be handled in a separate bug.
Attachment #604630 - Attachment is obsolete: true
Attachment #605734 - Flags: review+
Addressed review comments, carrying over r=bgirard. Will re-add r? mattwoodrow - I'm reasonably sure the transformations are correct, as getting them wrong would result in very obvious artifacing, but would be good to get confirmation.
Attachment #604631 - Attachment is obsolete: true
Attachment #605736 - Flags: review+
Attachment #604631 - Flags: review?(matt.woodrow)
Comment on attachment 605736 [details] [diff] [review]
Part 6 - Upload to texture on demand when double-buffering

Adding r? mattwoodrow to verify the transformations in ShadowThebesLayerOGL::RenderLayer are correct.
Attachment #605736 - Flags: review?(matt.woodrow)
Updated so as not to use AndroidBridge anymore - I didn't understand the comment about IPDL, but hopefully this addresses/circumvents that?
Attachment #604632 - Attachment is obsolete: true
Attachment #605739 - Flags: review?(bgirard)
Attachment #604626 - Flags: review?(bjacob) → review?(ajuma)
Attachment #604627 - Flags: review?(bjacob) → review?(ajuma)
Attachment #604628 - Flags: review?(bjacob) → review?(ajuma)
Attachment #604626 - Flags: review?(ajuma) → review+
Attachment #604627 - Flags: review?(ajuma) → review+
Comment on attachment 604628 [details] [diff] [review]
Part 3 - Use public TiledTextureImage iteration functions in DirectUpdate

Review of attachment 604628 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Attachment #604628 - Flags: review?(ajuma) → review+
Comment on attachment 605736 [details] [diff] [review]
Part 6 - Upload to texture on demand when double-buffering

Review of attachment 605736 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +967,5 @@
> +  if (aDelayUpload) {
> +    // Record the region that needs to be updated, and clip it to the size of
> +    // the texture.
> +    aPendingUploadRegion.Or(aPendingUploadRegion, destRegion).
> +      And(aPendingUploadRegion, nsIntRect(0, 0, size.width, size.height));

Probably easier to read if this was two separate lines.
Attachment #605736 - Flags: review?(matt.woodrow) → review+
Comment on attachment 605739 [details] [diff] [review]
Part 7 - Upload non-visible texture regions progressively

Review of attachment 605739 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, good work!

::: gfx/layers/ipc/CompositorParent.h
@@ +99,5 @@
>    void Destroy();
>  
>    LayerManager* GetLayerManager() { return mLayerManager; }
>  
> +  base::Thread* GetCompositorThread() { return mCompositorThread; }

As discussed we don't need this if we use MessageLoop::current(). Also MessageLoop::current() is more clear that your dispatching messages to the current context.

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +46,5 @@
>  #include "mozilla/ipc/SharedMemorySysV.h"
>  #include "mozilla/layers/PLayerChild.h"
>  #include "mozilla/layers/PLayersChild.h"
>  #include "mozilla/layers/PLayersParent.h"
> +#include "mozilla/layers/CompositorParent.h"

You don't need to include this, you forward declare this class anyways.

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +1144,5 @@
> +  mBuffer->EnsureTexture(mFrontBuffer.Buffer()->GetSize(),
> +                         mFrontBuffer.Buffer()->GetContentType());
> +  nsRefPtr<gl::TiledTextureImage> tiledImage =
> +    static_cast<gl::TiledTextureImage *>(mBuffer->GetTextureImage().get());
> +  tiledImage->SetIterationCallback(ProgressiveUploadCallback, (void *)&mRegionPendingUpload);

We should move IterationCallback to the base type. Callbacks would only be issued if the upload happens in step (TiledTextureImage). This will remove the cast and ifdef and simply the code.

@@ +1153,5 @@
> +  // Remove the iteration callback
> +  tiledImage->SetIterationCallback(nsnull, nsnull);
> +
> +  // Mark the task as completed
> +  mUploadTask = nsnull;

We could also Cancel() the task here before releasing it, but it's not required with your IsEmpty() check.

::: gfx/layers/opengl/ThebesLayerOGL.h
@@ +44,5 @@
>  #include "Layers.h"
>  #include "LayerManagerOGL.h"
>  #include "gfxImageSurface.h"
>  #include "GLContext.h"
> +#include "base/task.h"

Lets use 'class task;' to keep the size of the header down.

@@ +184,5 @@
>    // still requires uploading.
>    nsIntRegion mRegionPendingUpload;
>  
> +#ifdef MOZ_GFX_OPTIMIZE_MOBILE
> +  // Task used for progressive texture upload.

Nit: I would love to see a quick comment explaining the progressive upload feature here. Something like:
We use this task to postpone uploading area of the screen that are off the screen
Attachment #605739 - Flags: review?(bgirard) → review+
Adding refreshed patch - Part 1 became unused, so I've gotten rid of it. On BenWa's suggestion, I've added the tile iteration functions to the TextureImage base class.

Carrying forward r=ajuma
Attachment #604626 - Attachment is obsolete: true
Attachment #604627 - Attachment is obsolete: true
Attachment #606680 - Flags: review+
Rebased, carrying over r=ajuma.
Attachment #604628 - Attachment is obsolete: true
Attachment #606681 - Flags: review+
Carrying forward r=bgirard,mattwoodrow.

Changed to remove the #ifdef's, thanks to adding the tile iteration functions to TextureImage, and to enable double-buffering only when preferring small tiles.
Attachment #605736 - Attachment is obsolete: true
Attachment #606683 - Flags: review+
Carrying forward r=bgirard.

Uses MessageLoop::current() instead of adding stuff to LayerManager, removes #ifdefs.
Attachment #605739 - Attachment is obsolete: true
Attachment #606685 - Flags: review+
I'm going to go ahead and push this to inbound, a try run looks ok: https://tbpl.mozilla.org/?tree=Try&rev=78b103ba527c

I'll push in two parts - the first, up to removal of ShouldDoubleBuffer (this should all be pretty benign), then the rest. It'll at least make landing it again a bit easier if there are problems.
Depends on: 736850
Whiteboard: [gfx] → [gfx][QA+]
Comment on attachment 606683 [details] [diff] [review]
Part 6 - Upload to texture on demand when double-buffering

Review of attachment 606683 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +905,5 @@
>    void Upload(gfxASurface* aUpdate, const nsIntRegion& aUpdated,
> +              const nsIntRect& aRect, const nsIntPoint& aRotation,
> +              bool aDelayUpload, nsIntRegion& aPendingUploadRegion);
> +
> +  nsRefPtr<TextureImage> GetTextureImage() { return mTexImage; }

Why are you returning an nsRefPtr instead of a raw pointer here?
Depends on: 760834
You need to log in before you can comment on or make changes to this bug.