BasicCompositor (but not BasicCompositorX11) can efficiently use pixels in memory directly as a source with RGB-style formats, so the copy between BufferTextureHost and DataTextureSource is pretty wasteful. In this configuration BufferTextureClient/Host should be treated as a texture "without an internal buffer" so that it is used the same way Gralloc and D3D11 textures are used and provide a TextureSource that wraps the BufferTextureHost's memory directly.

This patch is implemented on top of bug 1249273.

Comment on attachment 8722497 [details] [diff] [review] Allow buffer textures to be treated as not having an internal buffer. Review of attachment 8722497 [details] [diff] [review]: ----------------------------------------------------------------- Naming of HasInternalBuffer seems to become confusing. The flag is changed by compositor type. Can't we rename it to another one like HasIntermediateBuffer?

(In reply to Sotaro Ikeda [:sotaro] from comment #2) > Comment on attachment 8722497 [details] [diff] [review] > Allow buffer textures to be treated as not having an internal buffer. > > Review of attachment 8722497 [details] [diff] [review]: > ----------------------------------------------------------------- > > Naming of HasInternalBuffer seems to become confusing. The flag is changed > by compositor type. Can't we rename it to another one like > HasIntermediateBuffer? HasIntermediateBuffer sounds like a better name, indeed. I filed bug 1250873.

Comment on attachment 8722497 [details] [diff] [review] Allow buffer textures to be treated as not having an internal buffer. Review of attachment 8722497 [details] [diff] [review]: ----------------------------------------------------------------- The patch seems good in general:) You might want to clean up codes around "hasInternalBuffer" handling. ::: gfx/layers/BufferTexture.cpp @@ +96,5 @@ > TextureAllocationFlags aAllocFlags, > ISurfaceAllocator* aAllocator) > { > + // TODO[nical] eeeek > + auto fwd = aAllocator ? static_cast<CompositableForwarder*>(aAllocator) : nullptr; Is there a case that aAllocator is nullptr? @@ +100,5 @@ > + auto fwd = aAllocator ? static_cast<CompositableForwarder*>(aAllocator) : nullptr; > + > + bool hasInternalBuffer = fwd > + ? fwd->GetCompositorBackendType() != LayersBackend::LAYERS_BASIC > + || aFormat == gfx::SurfaceFormat::YUV Do we need this check here? because CreateForYCbCr() should be used for YUV. MemoryTextureData::Create() and ShmemTextureData::Create() have "MOZ_ASSERT(aFormat != gfx::SurfaceFormat::YUV)" @@ +107,5 @@ > +#ifdef MOZ_WIDGET_GTK > + if (!hasInternalBuffer && gfxPlatformGtk::GetPlatform()->UseXRender()) { > + hasInternalBuffer = true; > + } > +#endif It might be better to split the above hasInternalBuffer decision part to a different function to make clear the code. @@ +418,5 @@ > MemoryTextureData::Create(gfx::IntSize aSize, gfx::SurfaceFormat aFormat, > gfx::BackendType aMoz2DBackend, TextureFlags aFlags, > TextureAllocationFlags aAllocFlags, > + ISurfaceAllocator*, > + bool aHasInternalBuffer) It seems like a bit wired to pass aHasInternalBuffer. It could be calculated internally. ::: gfx/layers/ipc/ISurfaceAllocator.cpp @@ +146,5 @@ > > bufferDesc = shmem; > } > > + *aBuffer = SurfaceDescriptorBuffer(RGBDescriptor(aSize, format, true), bufferDesc); It seems better to add a comment why it is true.

New version of the patch coming in a minute. (In reply to Sotaro Ikeda [:sotaro] from comment #4) > Comment on attachment 8722497 [details] [diff] [review] > Allow buffer textures to be treated as not having an internal buffer. > > + auto fwd = aAllocator ? static_cast<CompositableForwarder*>(aAllocator) : nullptr; > > Is there a case that aAllocator is nullptr? Currently only a gtest as far as I know. That test hasn't been any useful so I might just remove it someday but that's another story. > > @@ +100,5 @@ > > + auto fwd = aAllocator ? static_cast<CompositableForwarder*>(aAllocator) : nullptr; > > + > > + bool hasInternalBuffer = fwd > > + ? fwd->GetCompositorBackendType() != LayersBackend::LAYERS_BASIC > > + || aFormat == gfx::SurfaceFormat::YUV > > Do we need this check here? because CreateForYCbCr() should be used for YUV. > MemoryTextureData::Create() and ShmemTextureData::Create() have > "MOZ_ASSERT(aFormat != gfx::SurfaceFormat::YUV)" Right, I was being a over-cautious. I remved the check (and moved the whole thing as you suggest in another comment). > > It might be better to split the above hasInternalBuffer decision part to a > different function to make clear the code. > Updated the patch. > @@ +418,5 @@ > > MemoryTextureData::Create(gfx::IntSize aSize, gfx::SurfaceFormat aFormat, > > gfx::BackendType aMoz2DBackend, TextureFlags aFlags, > > TextureAllocationFlags aAllocFlags, > > + ISurfaceAllocator*, > > + bool aHasInternalBuffer) > > It seems like a bit wired to pass aHasInternalBuffer. It could be calculated > internally. I was initially trying to not duplicate this logic, but I moved the computation of hasInternalBuffer to its own function as you suggested, and now we call that function from the two constructors. > > > > + *aBuffer = SurfaceDescriptorBuffer(RGBDescriptor(aSize, format, true), bufferDesc); > > It seems better to add a comment why it is true. Updated the patch.

Comment on attachment 8723497 [details] [diff] [review] Updated patch Good!