Implement TextureImage::DirectUpload to avoid wasteful temporaries

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: cjones, Assigned: mattwoodrow)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+, fennec2.0b4+)

Details

Attachments

(7 attachments, 9 obsolete attachments)

520 bytes, patch
Details | Diff | Splinter Review
14.59 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
3.66 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
7.41 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
20.49 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
26.27 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
7.20 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
GL shadow layers that have backing surfaces currently have updates pushed to them as internally consistent shmem buffers (that is, the entire buffer contains valid content).  For these, going through TextureImage::BeginUpdate/EndUpdate can be unnecessarily expensive for cases where, e.g., the entire surface has changed, because we end up allocating a temporary image surface the size of the texture.  Various platform-specific texturing extensions may partially obsolete this code, but in the meantime, we should add a fast path for these cases that can avoid temporaries.  I propose this slightly modified version of bug 602428 comment 14

  bool TryDirectUpload(gfxImageSurface* aSurface, const nsIntRegion& aDirty)

which returns true if a direct glTex[Sub]Image2D was appropriate for the surface and dirty region (we can assume the teximage2d succeeded when this returns true), false otherwise.  This method could return false if, say, aDirty was a very small area of the texture, in which case allocating a temporary surface would be cheaper than a huge memcpy().

For some platform-specific texturing extensions, this may be the only upload path we take.

For shadow layers at first we can assume that aSurface is the same size as the texture and return false if that's not so.

I'll do quick back-of-the-envelope calculations to see when a big memcpy() will lose to a temporary surface, but that can be tuned.
Assignee: nobody → jmuizelaar
Created attachment 491338 [details] [diff] [review]
Prototype

This patch worked but currently has the wrong texture formats.
tracking-fennec: --- → ?
This is hurting us even more on mobile because we convert to 8888 from 565 before uploading. Unfortunately, it seems like the mobile 565 has the rgb in a different order than cairo does...
Really?  I thought there was only one way to do 565 and that cairo's and GL's matched.  You may be using the wrong set of fragment programs -- remember that we'll generally create textureimages with "isRGB" set to FALSE, because cairo's RGB byte order is opposite GL's when uploaded with GL_RGBA + GL_UNSIGNED_BYTES.  Then we use a fragment program that swaps R and B.  If you use 565, then isRGB needs to be TRUE, because we don't want the fragment program to swap R and B in that case.
(In reply to comment #3)
> Really?  I thought there was only one way to do 565 and that cairo's and GL's
> matched.  You may be using the wrong set of fragment programs -- remember that
> we'll generally create textureimages with "isRGB" set to FALSE, because cairo's
> RGB byte order is opposite GL's when uploaded with GL_RGBA + GL_UNSIGNED_BYTES.
>  Then we use a fragment program that swaps R and B.  If you use 565, then isRGB
> needs to be TRUE, because we don't want the fragment program to swap R and B in
> that case.

That was indeed the problem.
Created attachment 491926 [details] [diff] [review]
works with EGLTextureImage

This version works on my Samsung Galaxy S. It still needs cleanup as it assumes things that aren't necessarily always true.
Attachment #491338 - Attachment is obsolete: true

Updated

7 years ago
tracking-fennec: ? → 2.0b4+
(Assignee)

Comment 6

7 years ago
I feel we should have a single code path to upload a surface to a texture as efficiently as possible. Currently all the layers have separate implementations, as do all the TextureImage impls.

In bug 595180 I've added a 'UploadSurfaceToTexture' function that was intended to serve this purpose, but doing things through TextureImage would be fine to.

We should probably change the input param to be a gfxASurface and use GetAsImageSurface() to convert it, or do the slow copy if we have to. No point repeating that code everywhere.
(In reply to comment #6)
> I feel we should have a single code path to upload a surface to a texture as
> efficiently as possible. Currently all the layers have separate
> implementations, as do all the TextureImage impls.
> 
> In bug 595180 I've added a 'UploadSurfaceToTexture' function that was intended
> to serve this purpose, but doing things through TextureImage would be fine to.
> 
> We should probably change the input param to be a gfxASurface and use
> GetAsImageSurface() to convert it, or do the slow copy if we have to. No point
> repeating that code everywhere.

This sounds sane to me.
There are currently some problems with my current patch that I don't yet understand. When I tried to upload only mUpdateRect.width there seemed to be some uninitialized content so I changed it to mSize.width and that seemed to fix the problem.
(Assignee)

Updated

7 years ago
Blocks: 595180
(Assignee)

Comment 9

7 years ago
Created attachment 496608 [details] [diff] [review]
Add UploadSurfaceToTexture

This patch adds UploadSurfaceToTexture to LayerManagerOGL. I think it's generic enough that it can be used to implement both TextureImage::DirectUpload and all our normal layer upload code. This should more or less get all our texture uploading into a single location and make it easier to optimize (possibly using pixel buffer objects).

I haven't actually tested this yet, just putting it up for early feedback while I try changing code to use it.
Attachment #496608 - Flags: review?(jmuizelaar)
Comment on attachment 496608 [details] [diff] [review]
Add UploadSurfaceToTexture

This looks good so far.
(Assignee)

Updated

7 years ago
Attachment #496608 - Flags: review?(jmuizelaar)
(Assignee)

Comment 11

7 years ago
I've upload my entire patch queue to hold the work in progress to http://hg.mozilla.org/users/mwoodrow_mozilla.com/patch-queue

Relevant patches are UploadSurfaceToTexture, upload-* and pixel-buffer-objects.

These patches implement a centralized way to upload surface data to a texture, use it in CairoImageOGL, CanvasLayerOGL and BasicTextureImage and then attempt to use pixel buffer objects in BasicTextureImage.

Still needs a lot of cleaning up but appears to work so far, and performance looks good. Less than 0.5% CPU time spent within uploading thebes layers while scrolling like crazy
(Assignee)

Comment 12

7 years ago
I still need to look at implementing DirectUpload before this bug gets too off track.

A more difficult (and possibly more rewarding too!) patch would be to create pixel buffer objects for YCbCrImageOGL's and pass this to the decoder to write directly into.
(Assignee)

Comment 13

6 years ago
Created attachment 497644 [details] [diff] [review]
Add UploadSurfaceToTexture v2

Lot of fixes, moved into GLContext (since TextureImage doesn't know about LayerManagers).
Assignee: jmuizelaar → matt.woodrow+bugzilla
Attachment #491926 - Attachment is obsolete: true
Attachment #496608 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 14

6 years ago
Created attachment 497647 [details] [diff] [review]
Use UploadSurfaceToTexture in CairoImageOGL
(Assignee)

Comment 15

6 years ago
Created attachment 497648 [details] [diff] [review]
Use UploadSurfaceToTexture in CanvasLayerOGL
(Assignee)

Comment 16

6 years ago
Created attachment 497649 [details] [diff] [review]
Use UploadSurfaceToTexture in TextureImage
(Assignee)

Comment 17

6 years ago
Created attachment 497650 [details] [diff] [review]
Use Pixel Buffer Objects in BasicTextureImage
(Assignee)

Comment 18

6 years ago
Created attachment 497651 [details] [diff] [review]
Add TextureImage::DirectUpload

The above patch queue passes all reftests locally on mac. I've pushed it to try, will report back when I get results.

Entirely untested on mobile (DirectUpload!), Does anyone have a working mobile build configuration? I can't try this until I get home.
(Assignee)

Comment 19

6 years ago
Created attachment 497655 [details] [diff] [review]
Debugging patch - Enable OpenGL on mobile
Tryserver spits out mobile builds if you ask it.  If you need a special configuration, I might could help out.
Comment on attachment 497651 [details] [diff] [review]
Add TextureImage::DirectUpload

DirectUpdate might not be the best name. I didn't spend much time thinking about it when I chose it.
Attachment #497651 - Flags: review+
(Assignee)

Updated

6 years ago
Blocks: 611564
(Assignee)

Comment 22

6 years ago
Created attachment 498061 [details] [diff] [review]
Add UploadSurfaceToTexture v3

Tested and try server'd. More following!
Attachment #497644 - Attachment is obsolete: true
Attachment #497647 - Attachment is obsolete: true
Attachment #497648 - Attachment is obsolete: true
Attachment #497649 - Attachment is obsolete: true
Attachment #497650 - Attachment is obsolete: true
Attachment #497651 - Attachment is obsolete: true
Attachment #498061 - Flags: review?(joe)
(Assignee)

Comment 23

6 years ago
Created attachment 498062 [details] [diff] [review]
Use UploadSurfaceToTexture in CairoImageOGL v2
Attachment #498062 - Flags: review?(joe)
(Assignee)

Comment 24

6 years ago
Created attachment 498063 [details] [diff] [review]
Use UploadSurfaceToTexture in CanvasLayerOGL v2
Attachment #498063 - Flags: review?(joe)
(Assignee)

Comment 25

6 years ago
Created attachment 498064 [details] [diff] [review]
Use UploadSurfaceToTexture in TextureImage v2
Attachment #498064 - Flags: review?(joe)
(Assignee)

Comment 26

6 years ago
Created attachment 498065 [details] [diff] [review]
Use Pixel Buffer Objects in BasicTextureImage
Attachment #498065 - Flags: review?(joe)
(Assignee)

Comment 27

6 years ago
Created attachment 498066 [details] [diff] [review]
Add TextureImage::DirectUpload

Carrying forward r=jrmuizel (still waiting on a better name suggestion though).
Attachment #498066 - Flags: review+
Attachment #498061 - Flags: review?(joe) → review+
Attachment #498062 - Flags: review?(joe) → review+
Attachment #498063 - Flags: review?(joe) → review+
Attachment #498064 - Flags: review?(joe) → review+
(Assignee)

Updated

6 years ago
Depends on: 611443
Comment on attachment 498065 [details] [diff] [review]
Use Pixel Buffer Objects in BasicTextureImage

Don't check the return value of new!
Attachment #498065 - Flags: review?(joe) → review+
Comment on attachment 498065 [details] [diff] [review]
Use Pixel Buffer Objects in BasicTextureImage


>     updateSurface->SetDeviceOffset(gfxPoint(-mUpdateRect.x, -mUpdateRect.y));
> 
>     mUpdateContext = new gfxContext(updateSurface);
>+    

Add a comment like: Clear the buffer so that we can reuse it

> void
> BasicTextureImage::Resize(const nsIntSize& aSize)
> {
>     NS_ASSERTION(!mUpdateContext, "Resize() while in update?");
> 
>     mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture);
> 
>     mGLContext->fTexImage2D(LOCAL_GL_TEXTURE_2D,
>@@ -1203,17 +1238,18 @@ GLContext::BlitTextureImage(TextureImage
> }
> 
> 
> ShaderProgramType 
> GLContext::UploadSurfaceToTexture(gfxASurface *aSurface, 
>                                   const nsIntRect& aSrcRect,
>                                   GLuint& aTexture,
>                                   bool aOverwrite,
>-                                  const nsIntPoint& aDstPoint)
>+                                  const nsIntPoint& aDstPoint,
>+                                  bool aRelative)
> {
>   bool textureInited = aOverwrite ? false : true;
>   MakeCurrent();
>   fActiveTexture(LOCAL_GL_TEXTURE0);
>   
>   if (!aTexture) {
>     fGenTextures(1, &aTexture);
>     fBindTexture(LOCAL_GL_TEXTURE_2D, aTexture);
>@@ -1230,39 +1266,41 @@ GLContext::UploadSurfaceToTexture(gfxASu
>                    LOCAL_GL_TEXTURE_WRAP_T, 
>                    LOCAL_GL_CLAMP_TO_EDGE);
>     textureInited = false;
>   } else {
>     fBindTexture(LOCAL_GL_TEXTURE_2D, aTexture);
>   }
> 
>   nsRefPtr<gfxImageSurface> imageSurface = aSurface->GetAsImageSurface();
>-  unsigned char* data;
>+  unsigned char* data = NULL;
> 
>   if (!imageSurface || 
>       (imageSurface->Format() != gfxASurface::ImageFormatARGB32 &&
>        imageSurface->Format() != gfxASurface::ImageFormatRGB24 &&
>        imageSurface->Format() != gfxASurface::ImageFormatRGB16_565)) {
>     // We can't get suitable pixel data for the surface, make a copy
>     imageSurface = 
>       new gfxImageSurface(gfxIntSize(aSrcRect.width, aSrcRect.height), 
>                           gfxASurface::ImageFormatARGB32);
>   
>     nsRefPtr<gfxContext> context = new gfxContext(imageSurface);
> 
>     context->Translate(-gfxPoint(aSrcRect.x, aSrcRect.y));
>     context->SetSource(aSurface);
>     context->Paint();
>-    data = imageSurface->Data();
>   } else {
>-    data = imageSurface->Data();
>     data += aSrcRect.y * imageSurface->Stride();
>     data += aSrcRect.x * 4;
>   }
> 
>+  if (!aRelative) {
>+    data += (unsigned long)imageSurface->Data();
>+  }
>+

Let's move this above adding the offset.
i.e.

if (pixel_buffer) {
  data = NULL;
} else {
  data = imageSurface->Data();
}

>+        if (size > mPixelBufferSize) {
>+            mGLContext->fBufferData(LOCAL_GL_PIXEL_UNPACK_BUFFER, size,
>+                                    NULL, LOCAL_GL_STREAM_DRAW);
>+            mPixelBufferSize = size;
>+        }
>+        void *data = mGLContext->fMapBuffer(LOCAL_GL_PIXEL_UNPACK_BUFFER, LOCAL_GL_WRITE_ONLY);

Make the type of data unsigned char* also it's good to check the return value.
This blocks a blocker.
blocking2.0: --- → final+
http://hg.mozilla.org/mozilla-central/rev/ff4c041ae7de
http://hg.mozilla.org/mozilla-central/rev/e86ea759809b
http://hg.mozilla.org/mozilla-central/rev/c304184d7459
http://hg.mozilla.org/mozilla-central/rev/d8d69903f209
http://hg.mozilla.org/mozilla-central/rev/ac8edf791852
http://hg.mozilla.org/mozilla-central/rev/c5aeb066cbb5
http://hg.mozilla.org/mozilla-central/rev/d1da1005b6d6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 619934
This or bug 615741 caused a 3.77% Tp4 increase on MacOSX

On the positive side we got a 9.37%, 3.35% and 5.2% improvements on DHTML, tp4 and tscroll respectively.
For my not so well trained eye, this bug seems like the best one to blame for this crash: http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=mozilla%3A%3Agl%3A%3AGLContext%3A%3AUploadSurfaceToTexture&version=Firefox%3A4.0b9pre

Starting with Dec 17 nightly, I always get a crash on startup if layers.accelerate-all is set to true.

Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0b9pre) Gecko/20101216 Firefox/4.0b9pre ID:20101216030331
(In reply to comment #32)
> This or bug 615741 caused a 3.77% Tp4 increase on MacOSX

That's a Tp4 (RSS) regression, which is the measurement of resident set size (memory using) while running the pageload test.  It's not a speed regression (that was the 3.35% speed win on the same test).
(In reply to comment #33)
> For my not so well trained eye, this bug seems like the best one to blame for
> this crash:
> http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=mozilla%3A%3Agl%3A%3AGLContext%3A%3AUploadSurfaceToTexture&version=Firefox%3A4.0b9pre
>

This is fixed by bug 619934
(Assignee)

Comment 36

6 years ago
Not exactly sure what would cause the memory regression. We keep the pixel buffer memory alive, but I can't see how this differs from the old TextureImageCGL keeping mBackingSurface alive.

We could probably share one pixel buffer object between all instances of TextureImageCGL, since only one will use it at a time.
(Assignee)

Comment 37

6 years ago
More thoughts after a nights real sleep.

Texture memory usage will have increased because OpenGL still keeps a system memory copy of the full texture (same as we had before) and we're now also keeping some pixel buffer memory around as upload space.

Sharing pixel buffers between TextureImages won't work unless we flush drawing after each one.

We could free the stored pixel buffer memory every frame, on a timeout and/or consider passing different hint parameters to glBufferData. All of these are probably performance/memory usage trade-offs though.

Updated

6 years ago
Depends on: 620355

Updated

6 years ago
Depends on: 619933
Depends on: 634366

Updated

5 years ago
Depends on: 718199
You need to log in before you can comment on or make changes to this bug.