949573 - Use apple client storage with tiling

Status: RESOLVED DUPLICATE of bug 1265824

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Currently tiling upload underperforms compared to our current path. A quick prototype shows that apple client storage fixes this.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Part 1: Expose texture range

Comment 2

[Benoit Girard (:BenWa)]

https://developer.apple.com/library/mac/documentation/graphicsimaging/conceptual/opengl-macprogguide/opengl_texturedata/opengl_texturedata.html#//apple_ref/doc/uid/TP40001987-CH407-SW25

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Part 2: Use TEXTURE_RECTANGLE + client storage for tiling

Comment 4

[Chris Lord [:cwiiis]]

Comment on attachment 8346732 [details] [diff] [review]
Part 2: Use TEXTURE_RECTANGLE + client storage for tiling

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

Fly-by review. I'm curious as to whether the replacing of LOCAL_GL_TEXTURE_2D with LOCAL_GL_TEXTURE_RECTANGLE_ARB will cause problems on Android - does this patch change talos numbers for Android at all? Of course, it shouldn't, but that may be wishful thinking.

::: gfx/layers/client/TextureClient.cpp
@@ +23,5 @@
>  #include "nsTraceRefcnt.h"              // for MOZ_COUNT_CTOR, etc
>  #include "ImageContainer.h"             // for PlanarYCbCrImage, etc
>  #include "mozilla/gfx/2D.h"
>  
> +// inprocess tiles performs better and does use shmem

s/does/doesn't/, I assume?

@@ +637,1 @@
>      // If we're using OMTC, we can save some cycles by not using shared

This comment should be changed, or the #ifdef will be confusing out of context.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +991,5 @@
> +        // ensuring it's safe to release it.
> +        mBoundTextureData->ReadUnlock();
> +        mBoundTextureData = nullptr;
> +      } else {
> +        gl::GfxTexturesReporter::UpdateAmount(gl::GfxTexturesReporter::MemoryFreed,

Could just be splinter, but this indentation doesn't look right?

@@ +1006,5 @@
> +      } else {
> +        // if MakeCurrent failed because the context was already destoyed, it means
> +        // the driver already freed the texture memory underneith us, so it should
> +        // not count as a leak.
> +        gl::GfxTexturesReporter::UpdateAmount(gl::GfxTexturesReporter::MemoryFreed,

and same here.

@@ +1024,5 @@
>    if (!mGL->MakeCurrent()) {
>      return;
>    }
>  
> +  GLenum target = LOCAL_GL_TEXTURE_RECTANGLE_ARB;

This changes the target from what was previously LOCAL_GL_TEXTURE_2D - is this intentional? Could this have side-effects on crappier drivers?

@@ +1027,5 @@
>  
> +  GLenum target = LOCAL_GL_TEXTURE_RECTANGLE_ARB;
> +  bool useAppleClientStorage = false;
> +#ifdef XP_MACOSX
> +  useAppleClientStorage = true;

Could this just check if the extension is available, rather than #ifdef?

@@ +1046,5 @@
> +    mGL->fBindTexture(target, mTextureHandle);
> +    if (!useAppleClientStorage) {
> +      // We're re-using a texture, but the format may change. Update the memory
> +      // reporter with a free and alloc (below) using the old and new formats.
> +      gl::GfxTexturesReporter::UpdateAmount(gl::GfxTexturesReporter::MemoryFreed,

ok, I guess this indentation is intentional... Weird.

@@ +1063,5 @@
> +    if (mBoundTextureData) {
> +      mBoundTextureData->ReadUnlock();
> +    }
> +    int len = TILEDLAYERBUFFER_TILE_SIZE * TILEDLAYERBUFFER_TILE_SIZE *
> +              mGLFormat == LOCAL_GL_RGB ? 2 : 4;

I assume this is some kind of Mac-ism, RGB is always 16-bit...?

Also, not a fan of ternarys without parenthesis, personally. (i.e. around 'mGLFormat == LOCAL_GL_RGB')

@@ +1079,5 @@
> +  }
> +
> +  mGL->fTexImage2D(target, 0, mGLFormat,
> +                     TILEDLAYERBUFFER_TILE_SIZE, TILEDLAYERBUFFER_TILE_SIZE, 0,
> +                     format, type, buf);

The indentation here is off, where previously it was correct.

::: gfx/layers/opengl/TextureHostOGL.h
@@ +824,5 @@
>  
>    gfx::IntSize mSize;
>    GLuint mTextureHandle;
>    GLenum mGLFormat;
> +  // Client storage may keep a read lock

s/Client storage/Apple client storage/ ?

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Part 2: Use TEXTURE_RECTANGLE + client storage for tiling

Comment 6

[Chris Lord [:cwiiis]]

Comment on attachment 8346820 [details] [diff] [review]
Part 2: Use TEXTURE_RECTANGLE + client storage for tiling

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

LGTM from an Android perspective - can't comment on the Mac stuff, but I'd say that looks good too.

Comment 7

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=475d575d10fe

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 8346732 [details] [diff] [review]
Part 2: Use TEXTURE_RECTANGLE + client storage for tiling

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

Opps my comments never got posted. You were meant to see this before. Let me know if you're not happy with anything I left un-addressed.

::: gfx/layers/client/TextureClient.cpp
@@ +637,1 @@
>      // If we're using OMTC, we can save some cycles by not using shared

What is confusing about the comments? I added in process but I think it's a good description. Let me know.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +991,5 @@
> +        // ensuring it's safe to release it.
> +        mBoundTextureData->ReadUnlock();
> +        mBoundTextureData = nullptr;
> +      } else {
> +        gl::GfxTexturesReporter::UpdateAmount(gl::GfxTexturesReporter::MemoryFreed,

Looks fine. Perhaps your window is too small?

@@ +1024,5 @@
>    if (!mGL->MakeCurrent()) {
>      return;
>    }
>  
> +  GLenum target = LOCAL_GL_TEXTURE_RECTANGLE_ARB;

Jeff tells me it's not supported on OGL es. It's now ifdefs for mac only.

@@ +1027,5 @@
>  
> +  GLenum target = LOCAL_GL_TEXTURE_RECTANGLE_ARB;
> +  bool useAppleClientStorage = false;
> +#ifdef XP_MACOSX
> +  useAppleClientStorage = true;

I could but I also wanted the compiler to throw out this code. =\

@@ +1063,5 @@
> +    if (mBoundTextureData) {
> +      mBoundTextureData->ReadUnlock();
> +    }
> +    int len = TILEDLAYERBUFFER_TILE_SIZE * TILEDLAYERBUFFER_TILE_SIZE *
> +              mGLFormat == LOCAL_GL_RGB ? 2 : 4;

Tiles only supports RGB565 and RGB8888.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8346820 [details] [diff] [review]
Part 2: Use TEXTURE_RECTANGLE + client storage for tiling

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

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +944,5 @@
> +    return FORMAT_R8G8B8A8;
> +  } else {
> +    return FORMAT_R5G6B5;
> +  }
> +}

This is wrong. We should be returning mFormat.

@@ +1001,5 @@
> +        // If we're using client storage the delete call
> +        // will block until the texture is no longer in use
> +        // ensuring it's safe to release it.
> +        mBoundTextureData->ReadUnlock();
> +        mBoundTextureData = nullptr;

It's sort of shame to duplicate this chunk of code.

@@ +1073,5 @@
> +    }
> +    mBoundTextureData = aReusableSurface;
> +    mBoundTextureData->ReadLock();
> +    mGL->fTextureRangeAPPLE(target, len, const_cast<unsigned char*>(buf));
> +    mGL->fPixelStorei(LOCAL_GL_UNPACK_CLIENT_STORAGE_APPLE, LOCAL_GL_TRUE);

Please add a comment about how both of these are needed and why.

::: gfx/layers/opengl/TextureHostOGL.h
@@ +828,5 @@
>  
>    gfx::IntSize mSize;
>    GLuint mTextureHandle;
>    GLenum mGLFormat;
> +  // Apple client storage may keep a read lock

This comment is pretty ambiguous. Can you clarify what it means and explain why the member needs to be there.

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 8346675 [details] [diff] [review]
Part 1: Expose texture range

Landed in bug 971154

Comment 11

[Florian Bender]

Benoit, still working on this? You set the state back to NEW but did not unassign yourself.

Comment 12

[Benoit Girard (:BenWa)]

No, opps, not at the moment. This patch needs to be re-written because the code has changed. Since we're working on tiling for desktop this may need to get picked up this quarter if we see upload regressions, we will see.

Comment 13

[Florian Bender]

It looks like Tiling is now enabled on Mac, at least in Nightlies. Is this bug still wanted?

Comment 14

[Benoit Girard (:BenWa)]

Yes

Comment 15

[Jeff Muizelaar [:jrmuizel]]

We're going to make another attempt at this in bug 1265824