1243072 - Track OpenGL texture usage per context and globally

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

We don't seem to have any easy way to know how much texture memory we're using. This would be pretty good to have, especially on mobile (Android).

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Make GfxTexturesReporter work again

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8712358 [details] [diff] [review]
Make GfxTexturesReporter work again

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

::: gfx/gl/GLTextureImage.cpp
@@ +183,5 @@
>      RefPtr<gfx::SourceSurface> updateSnapshot = mUpdateDrawTarget->Snapshot();
>      RefPtr<gfx::DataSourceSurface> updateData = updateSnapshot->GetDataSurface();
>  
>      bool relative = FinishedSurfaceUpdate();
> +    size_t uploadSize = 0;

If the outparam is infallible, you don't need to zero it. You may, but I don't require it.

::: gfx/gl/GLUploadHelpers.cpp
@@ +380,5 @@
>      }
>  }
>  
> +static uint32_t
> +GetBitsPerTexel(GLenum format, GLenum type)

Why is this bits? Why not bytes? Prefer bytes to bits unless there's a reason.

@@ +411,5 @@
> +            case LOCAL_GL_RGBA:
> +                return 4 * multiplier;
> +            case LOCAL_GL_COMPRESSED_RGB_PVRTC_2BPPV1:
> +            case LOCAL_GL_COMPRESSED_RGBA_PVRTC_2BPPV1:
> +                return 2;

Ah, so this is why. A few problems:
* This isn't really accurate for non-block-aligned texture sizes, which are valid for a number of these. (But are we actually storing these formats ever?)
* ASTC is going to be pretty popular fairly soon, and it doesn't have integer bit-per-pixel values.

Most importantly though:
* COMPRESSED_ enums are `sizedFormat`s that are used with CompressedTex*Image, not Tex*Image, and they do not have an accompanying `type`.

Strip all the compressed format handling from here, change this function to deal with bytes, and MOZ_RELEASE_ASSERT if we don't recognize the format and/or type. I see we have a MOZ_ASSERT, but I don't believe these give sufficient coverage, whereas having an easily-fixable nightly crash will guarantee we're not missing things.

@@ +578,5 @@
>      // Top left point of the region's bounding rectangle.
>      IntPoint topLeft = paintRegion.GetBounds().TopLeft();
>  
> +    if (*aUploadSize) {
> +      *aUploadSize = 0;

Either:
if (aUploadSize) {
  *aUploadSize = 0;
}
Or:
*aUploadSize = 0;

@@ +620,5 @@
>                               rectData);
>          }
>  
> +        if (aUploadSize && !textureInited) {
> +          *aUploadSize += int64_t(GetBitsPerTexel(internalFormat, type)) *

Why do you cast this to int64_t when it's non-negative and is going to be stored as a size_t anyway?

@@ +621,5 @@
>          }
>  
> +        if (aUploadSize && !textureInited) {
> +          *aUploadSize += int64_t(GetBitsPerTexel(internalFormat, type)) *
> +            int64_t(iterRect->width) * int64_t(iterRect->height);

If aUploadSize is in bits, it should be aUploadSize{In}Bits or aUploadedBits/BitCount.

@@ +633,5 @@
>  UploadSurfaceToTexture(GLContext* gl,
>                         DataSourceSurface *aSurface,
>                         const nsIntRegion& aDstRegion,
>                         GLuint& aTexture,
> +                       size_t *aUploadSize,

Star-to-left. Feel free to correct aSurface while you're here.

::: gfx/gl/GLUploadHelpers.h
@@ +39,5 @@
>    *
>    * \param aData Image data to upload.
>    * \param aDstRegion Region of texture to upload to.
>    * \param aTexture Texture to use, or 0 to have one created for you.
> +  * \param aUplaodSize if set, the number of bytes the texture requires will be returned here

s/Uplaod/Upload/

@@ +59,5 @@
>                           int32_t aStride,
>                           gfx::SurfaceFormat aFormat,
>                           const nsIntRegion& aDstRegion,
>                           GLuint& aTexture,
> +                         size_t *aUploadSize = nullptr,

size_t* {const} a{Out}UploadSize

@@ +73,5 @@
>  UploadSurfaceToTexture(GLContext* gl,
>                         gfx::DataSourceSurface *aSurface,
>                         const nsIntRegion& aDstRegion,
>                         GLuint& aTexture,
> +                       size_t *aUploadSize = nullptr,

size_t* {const} a{Out}UploadSize

::: gfx/gl/GfxTexturesReporter.cpp
@@ +20,2 @@
>      if (action == MemoryFreed) {
> +        sAmount -= amount;

MOZ_ASSERT(amount <= sAmount)

::: gfx/gl/GfxTexturesReporter.h
@@ +47,1 @@
>        sTileWasteAmount += delta;

sTileWasteAmount monotonically increases?

@@ +58,5 @@
>              "Memory used for storing GL textures.");
>      }
>  
>  private:
> +    static Atomic<size_t> sAmount;

I'm not super-enthused about Atomic<size_t>, but if it works, this is fine.

GPU allocs should be rare enough that locking a Mutex is acceptable. If not, we're optimizing the wrong thing by going with Atomics.

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8712358 [details] [diff] [review]
Make GfxTexturesReporter work again

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

r=me with Jeff's comments addressed.
I personally prefer using atomics rather than locks when just incrementing a counter, since I find it simpler in every aspect. Not sure what's bothering Jeff for this one.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
<snip lots of good comments>
> >  
> > +        if (aUploadSize && !textureInited) {
> > +          *aUploadSize += int64_t(GetBitsPerTexel(internalFormat, type)) *
> > +            int64_t(iterRect->width) * int64_t(iterRect->height);
> 
> If aUploadSize is in bits, it should be aUploadSize{In}Bits or
> aUploadedBits/BitCount.

Oops. I just moved this code from GfxTexturesReporter and didn't even realize it was in bits (because it doesn't make sense). That explains the ridiculously high numbers I was getting!

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Make GfxTexturesReporter work again with OpenGL

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8712727 [details] [diff] [review]
Make GfxTexturesReporter work again with OpenGL

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

::: gfx/gl/GLTextureImage.cpp
@@ +99,5 @@
> +void
> +TextureImage::UpdateUploadSize(size_t amount)
> +{
> +    if (amount == 0) {
> +        return;

Doesn't this prevent us from clearing the value without SubtractUploadSize?

If you drop this early-out, you can remove SubtractUploadSize entirely.

::: gfx/gl/GLUploadHelpers.cpp
@@ +553,5 @@
>      // Top left point of the region's bounding rectangle.
>      IntPoint topLeft = paintRegion.GetBounds().TopLeft();
>  
> +    if (aOutUploadSize) {
> +      *aOutUploadSize = 0;

This is a 4-space file.

@@ +596,5 @@
>          }
>  
> +        if (aOutUploadSize && !textureInited) {
> +          *aOutUploadSize += GetBytesPerTexel(internalFormat, type) *
> +            size_t(iterRect->width) * size_t(iterRect->height);

This is a 4-space file.

Also for long lines like this, prefer to create named local vars to try to fit it on one line.

::: gfx/gl/GfxTexturesReporter.cpp
@@ +20,2 @@
>      if (action == MemoryFreed) {
> +        MOZ_ASSERT(amount < sAmount);

Consider MOZ_RELEASE_ASSERT for this.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de75dce16ee4

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d02a1e213646

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3e1c0c851f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b3e1c0c851f