732917 - MAPLE: Texture upload causes significant pauses during panning/flinging

Status: RESOLVED FIXED

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

Description

[Chris Lord [:cwiiis]]

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.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Add push/pop for tile iteration

This patch adds push/pop functions to TiledTextureImage, allowing for recursive tile iteration.

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Add a callback for tile iteration

This patch adds an optional callback on TiledTextureImage that gets called on tile iteration (practically, when NextTile is called).

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Use public TiledTextureImage iteration functions in TiledTextureImage::DirectUpdate

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.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Part X - Spin compositor message loop while tile upload is happening

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!

Comment 5

[Chris Lord [:cwiiis]]

Ccing people I think may be interested.

Comment 6

[Ali Juma [:ajuma]]

(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.

Comment 7

[Chris Lord [:cwiiis]]

Ok, I'm going to have a go at implementing option 1.

Comment 8

[Benoit Girard (:BenWa)]

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 9

[Benoit Girard (:BenWa)]

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.

Comment 10

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Remove ShadowLayers::ShouldDoubleBuffer

ShadowLayers::ShouldDoubleBuffer has been unused since bug 690469.

Comment 11

[Chris Lord [:cwiiis]]

Attachment: Part 5 - Enable double-buffering on adreno/android

This enables double-buffering on the content side for ShadowThebesLayerGL on Android/Adreno, as a build-up to progressive texture upload.

Comment 12

[Chris Lord [:cwiiis]]

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)

Comment 13

[Chris Lord [:cwiiis]]

While I still appreciate help here, I think I've cracked how to get the intersecting rectangle. Will hopefully finish up this part tomorrow.

Comment 14

[Joe Drew (not getting mail)]

Timothy or Mats might be able to help with your questions in comment 12.

Comment 15

[Chris Lord [:cwiiis]]

Attachment: Part 6 - Upload to texture on demand

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.

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Part 7 - Upload progressively, asynchronously

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.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Add store/restore for recursive tile iteration

Comment 18

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Add a callback for tile iteration

Comment 19

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Use public TiledTextureImage iteration functions in DirectUpdate

Comment 20

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Remove ShadowLayers::ShouldDoubleBuffer

Comment 21

[Chris Lord [:cwiiis]]

Attachment: Part 5 - Enable double-buffering on adreno/android

Comment 22

[Chris Lord [:cwiiis]]

Attachment: Part 6 - Upload to texture on demand when double-buffering

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).

Comment 23

[Chris Lord [:cwiiis]]

Attachment: Part 7 - Upload progressively, asynchronously

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.

Comment 24

[Ali Juma [:ajuma]]

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.

Comment 25

[Benoit Girard (:BenWa)]

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.

Comment 26

[Chris Lord [:cwiiis]]

(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)

Comment 27

[Chris Lord [:cwiiis]]

(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.

Comment 28

[Chris Lord [:cwiiis]]

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.

Comment 29

[Benoit Girard (:BenWa)]

Comment on attachment 604632 [details] [diff] [review]
Part 7 - Upload progressively, asynchronously

I think what you want is to send a real IPDL message.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 604629 [details] [diff] [review]
Part 4 - Remove ShadowLayers::ShouldDoubleBuffer

># HG changeset patch
># Parent 6f9022834aa261f0131787458f11bdf330be28cb
># User Chris Lord <[e-mail]>
>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

Comment 31

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Remove ShadowLayers::ShouldDoubleBuffer

Addressed comment, carrying over r=cjones.

Comment 32

[Chris Lord [:cwiiis]]

Attachment: Part 5 - Enable double-buffering on Android

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.

Comment 33

[Chris Lord [:cwiiis]]

Attachment: Part 6 - Upload to texture on demand when double-buffering

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.

Comment 34

[Chris Lord [:cwiiis]]

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.

Comment 35

[Chris Lord [:cwiiis]]

Attachment: Part 7 - Upload non-visible texture regions progressively

Updated so as not to use AndroidBridge anymore - I didn't understand the comment about IPDL, but hopefully this addresses/circumvents that?

Comment 36

[Ali Juma [:ajuma]]

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.

Comment 37

[Matt Woodrow (:mattwoodrow)]

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.

Comment 38

[Benoit Girard (:BenWa)]

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

Comment 39

[Chris Lord [:cwiiis]]

Attachment: Part 1/2 - Add a callback for tile iteration

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

Comment 40

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Use public TiledTextureImage iteration functions in DirectUpdate

Rebased, carrying over r=ajuma.

Comment 41

[Chris Lord [:cwiiis]]

Attachment: Part 6 - Upload to texture on demand when double-buffering

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.

Comment 42

[Chris Lord [:cwiiis]]

Attachment: Part 7 - Upload non-visible texture regions progressively

Carrying forward r=bgirard.

Uses MessageLoop::current() instead of adding stuff to LayerManager, removes #ifdefs.

Comment 43

[Chris Lord [:cwiiis]]

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.

Comment 44

[Chris Lord [:cwiiis]]

Pushed to inbound.

Push 1: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7945310b462c
Push 2: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=bfc040abada9

Comment 45

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bfc040abada9
https://hg.mozilla.org/mozilla-central/rev/454764585486
https://hg.mozilla.org/mozilla-central/rev/307d7efc6725
https://hg.mozilla.org/mozilla-central/rev/7945310b462c
https://hg.mozilla.org/mozilla-central/rev/5ae83c00013c
https://hg.mozilla.org/mozilla-central/rev/8c0aa70d84bf

Comment 46

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?