Closed
Bug 1250500
Opened 8 years ago
Closed 8 years ago
Avoid copying BufferTextureHost if the compositor can sample directly from its memory
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: nical, Assigned: nical)
References
Details
Attachments
(1 file, 1 obsolete file)
19.72 KB,
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
This patch is implemented on top of bug 1249273.
Attachment #8722497 -
Flags: review?(sotaro.ikeda.g)
Comment 2•8 years ago
|
||
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?
Assignee | ||
Comment 3•8 years ago
|
||
(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 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8722497 -
Attachment is obsolete: true
Attachment #8722497 -
Flags: review?(sotaro.ikeda.g)
Attachment #8723497 -
Flags: review?(sotaro.ikeda.g)
Comment 7•8 years ago
|
||
Comment on attachment 8723497 [details] [diff] [review] Updated patch Good!
Attachment #8723497 -
Flags: review?(sotaro.ikeda.g) → review+
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/52e355f34371
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•