commit 3bf86e525eff6fb01e4c160dd8708100f05f70ad
parent ab8461329f5c76c42ff47b9a1b6dc9548756dcf8
Author: Atila Butkovits <abutkovits@mozilla.com>
Date: Wed, 26 Nov 2025 10:04:56 +0200
Revert "Bug 2002342 - Reduce CanvasTranslator data shmem copies. r=aosmond" for causing failures at test_animSVGImage.html.
This reverts commit 43ae770183e0d8922ba5b6d317e841a8b58aa246.
Diffstat:
7 files changed, 48 insertions(+), 184 deletions(-)
diff --git a/gfx/layers/RecordedCanvasEventImpl.h b/gfx/layers/RecordedCanvasEventImpl.h
@@ -295,10 +295,8 @@ RecordedCacheDataSurface::RecordedCacheDataSurface(S& aStream)
class RecordedGetDataForSurface final
: public RecordedEventDerived<RecordedGetDataForSurface> {
public:
- RecordedGetDataForSurface(uint32_t aId, const gfx::SourceSurface* aSurface)
- : RecordedEventDerived(GET_DATA_FOR_SURFACE),
- mId(aId),
- mSurface(aSurface) {}
+ explicit RecordedGetDataForSurface(const gfx::SourceSurface* aSurface)
+ : RecordedEventDerived(GET_DATA_FOR_SURFACE), mSurface(aSurface) {}
template <class S>
MOZ_IMPLICIT RecordedGetDataForSurface(S& aStream);
@@ -311,26 +309,23 @@ class RecordedGetDataForSurface final
std::string GetName() const final { return "RecordedGetDataForSurface"; }
private:
- uint32_t mId = 0;
ReferencePtr mSurface;
};
inline bool RecordedGetDataForSurface::PlayCanvasEvent(
CanvasTranslator* aTranslator) const {
- aTranslator->GetDataSurface(mId, mSurface.mLongPtr);
+ aTranslator->GetDataSurface(mSurface.mLongPtr);
return true;
}
template <class S>
void RecordedGetDataForSurface::Record(S& aStream) const {
- WriteElement(aStream, mId);
WriteElement(aStream, mSurface);
}
template <class S>
RecordedGetDataForSurface::RecordedGetDataForSurface(S& aStream)
: RecordedEventDerived(GET_DATA_FOR_SURFACE) {
- ReadElement(aStream, mId);
ReadElement(aStream, mSurface);
}
diff --git a/gfx/layers/ipc/CanvasChild.cpp b/gfx/layers/ipc/CanvasChild.cpp
@@ -552,12 +552,7 @@ bool CanvasChild::EnsureDataSurfaceShmem(size_t aSizeRequired) {
return false;
}
- auto id = ++mNextDataSurfaceShmemId;
- if (!id) {
- // If ids overflow, ensure that zero is reserved.
- id = ++mNextDataSurfaceShmemId;
- }
- if (!SendSetDataSurfaceBuffer(id, std::move(shmemHandle))) {
+ if (!SendSetDataSurfaceBuffer(std::move(shmemHandle))) {
return false;
}
@@ -653,13 +648,19 @@ already_AddRefed<gfx::DataSourceSurface> CanvasChild::GetDataSurface(
return nullptr;
}
- RecordEvent(RecordedCacheDataSurface(aSurface));
+ // If growing the data surface shmem, allocation may require significant time
+ // in the content process, during which the GPU process may issue an earlier
+ // readback while the content process is still busy. If the existing data
+ // surface shmem is to be reused instead, then try to instead read the data
+ // directly into the shmem to avoid a superfluous copy after readback.
+ bool forceData = ShouldGrowDataSurfaceShmem(sizeRequired);
+ RecordEvent(RecordedCacheDataSurface(aSurface, forceData));
if (!EnsureDataSurfaceShmem(sizeRequired)) {
return nullptr;
}
- RecordEvent(RecordedGetDataForSurface(mNextDataSurfaceShmemId, aSurface));
+ RecordEvent(RecordedGetDataForSurface(aSurface));
auto checkpoint = CreateCheckpoint();
if (NS_WARN_IF(!mRecorder->WaitForCheckpoint(checkpoint))) {
return nullptr;
diff --git a/gfx/layers/ipc/CanvasChild.h b/gfx/layers/ipc/CanvasChild.h
@@ -197,7 +197,6 @@ class CanvasChild final : public PCanvasChild, public SupportsWeakPtr {
std::shared_ptr<ipc::ReadOnlySharedMemoryMapping> mDataSurfaceShmem;
bool mDataSurfaceShmemAvailable = false;
- uint32_t mNextDataSurfaceShmemId = 0;
int64_t mLastWriteLockCheckpoint = 0;
uint32_t mTransactionsSinceGetDataSurface = kCacheDataSurfaceThreshold;
struct TextureInfo {
diff --git a/gfx/layers/ipc/CanvasTranslator.cpp b/gfx/layers/ipc/CanvasTranslator.cpp
@@ -284,7 +284,7 @@ bool CanvasTranslator::AddBuffer(
}
ipc::IPCResult CanvasTranslator::RecvSetDataSurfaceBuffer(
- uint32_t aId, ipc::MutableSharedMemoryHandle&& aBufferHandle) {
+ ipc::MutableSharedMemoryHandle&& aBufferHandle) {
if (mDeactivated) {
// The other side might have sent a resume message before we deactivated.
return IPC_OK();
@@ -293,77 +293,29 @@ ipc::IPCResult CanvasTranslator::RecvSetDataSurfaceBuffer(
if (UsePendingCanvasTranslatorEvents()) {
MutexAutoLock lock(mCanvasTranslatorEventsLock);
mPendingCanvasTranslatorEvents.push_back(
- CanvasTranslatorEvent::SetDataSurfaceBuffer(aId,
- std::move(aBufferHandle)));
+ CanvasTranslatorEvent::SetDataSurfaceBuffer(std::move(aBufferHandle)));
PostCanvasTranslatorEvents(lock);
} else {
- DispatchToTaskQueue(
- NewRunnableMethod<uint32_t, ipc::MutableSharedMemoryHandle&&>(
- "CanvasTranslator::SetDataSurfaceBuffer", this,
- &CanvasTranslator::SetDataSurfaceBuffer, aId,
- std::move(aBufferHandle)));
+ DispatchToTaskQueue(NewRunnableMethod<ipc::MutableSharedMemoryHandle&&>(
+ "CanvasTranslator::SetDataSurfaceBuffer", this,
+ &CanvasTranslator::SetDataSurfaceBuffer, std::move(aBufferHandle)));
}
return IPC_OK();
}
-void CanvasTranslator::DataSurfaceBufferWillChange(uint32_t aId,
- bool aKeepAlive,
- size_t aLimit) {
- if (aId) {
- // If a specific id is changing, then attempt to copy it.
- auto it = mDataSurfaceShmems.find(aId);
- if (it != mDataSurfaceShmems.end()) {
- RefPtr<gfx::DataSourceSurface> owner(it->second.mOwner);
- if (owner) {
- // Force copy-on-write of contained shmem if applicable
- gfx::DataSourceSurface::ScopedMap map(
- owner, gfx::DataSourceSurface::MapType::READ_WRITE);
- it->second.mOwner = nullptr;
- }
- // Erase the shmem if it's not the last used id.
- if (!aKeepAlive || aId != mLastDataSurfaceShmemId) {
- mDataSurfaceShmems.erase(it);
- }
- }
- } else {
- // If no limit is requested, clear everything.
- if (!aLimit) {
- aLimit = mDataSurfaceShmems.size();
- }
- // Ensure all shmems still alive are copied.
- DataSurfaceShmem lastShmem;
- auto it = mDataSurfaceShmems.begin();
- for (; aLimit > 0 && it != mDataSurfaceShmems.end(); ++it, --aLimit) {
- // If the last shmem id needs to be kept alive, move it before clearing
- // the hashtable.
- if (aKeepAlive && it->first == mLastDataSurfaceShmemId) {
- lastShmem = std::move(it->second);
- continue;
- }
- RefPtr<gfx::DataSourceSurface> owner(it->second.mOwner);
- if (owner) {
- // Force copy-on-write of contained shmem if applicable
- gfx::DataSourceSurface::ScopedMap map(
- owner, gfx::DataSourceSurface::MapType::READ_WRITE);
- it->second.mOwner = nullptr;
- }
- }
- // Clear all iterated shmem mappings.
- if (it == mDataSurfaceShmems.end()) {
- mDataSurfaceShmems.clear();
- } else if (it != mDataSurfaceShmems.begin()) {
- mDataSurfaceShmems.erase(mDataSurfaceShmems.begin(), it);
- }
- // Restore the last shmem id if necessary.
- if (lastShmem.mShmem.IsValid()) {
- mDataSurfaceShmems[mLastDataSurfaceShmemId] = std::move(lastShmem);
- }
+void CanvasTranslator::DataSurfaceBufferWillChange() {
+ RefPtr<gfx::DataSourceSurface> owner(mDataSurfaceShmemOwner);
+ if (owner) {
+ // Force copy-on-write of contained shmem if applicable
+ gfx::DataSourceSurface::ScopedMap map(
+ owner, gfx::DataSourceSurface::MapType::READ_WRITE);
+ mDataSurfaceShmemOwner = nullptr;
}
}
bool CanvasTranslator::SetDataSurfaceBuffer(
- uint32_t aId, ipc::MutableSharedMemoryHandle&& aBufferHandle) {
+ ipc::MutableSharedMemoryHandle&& aBufferHandle) {
MOZ_ASSERT(IsInTaskQueue());
if (mHeader->readerState == State::Failed) {
// We failed before we got to the pause event.
@@ -380,53 +332,16 @@ bool CanvasTranslator::SetDataSurfaceBuffer(
return false;
}
- if (!aId) {
+ DataSurfaceBufferWillChange();
+ mDataSurfaceShmem = aBufferHandle.Map();
+ if (!mDataSurfaceShmem) {
return false;
}
- if (aId < mLastDataSurfaceShmemId) {
- // The id space has overflowed, so clear out all existing mapping.
- DataSurfaceBufferWillChange(0, false);
- } else if (mLastDataSurfaceShmemId != aId) {
- // The last shmem id will be kept alive, even if there is no owner. The last
- // shmem id is about to change, so if the current last id has no owner, it
- // needs to be erased.
- auto it = mDataSurfaceShmems.find(mLastDataSurfaceShmemId);
- if (it != mDataSurfaceShmems.end() && it->second.mOwner.IsDead()) {
- mDataSurfaceShmems.erase(it);
- }
- // Ensure the current shmems don't exceed the maximum allowed.
- size_t maxShmems = StaticPrefs::gfx_canvas_accelerated_max_data_shmems();
- if (maxShmems > 0 && mDataSurfaceShmems.size() >= maxShmems) {
- DataSurfaceBufferWillChange(0, false,
- (mDataSurfaceShmems.size() - maxShmems) + 1);
- }
- }
- mLastDataSurfaceShmemId = aId;
-
- // If the id is being reused, then ensure the old contents is copied before
- // the buffer is changed.
- DataSurfaceBufferWillChange(aId);
-
- // Finally, change the shmem mapping.
- {
- auto& dataSurfaceShmem = mDataSurfaceShmems[aId];
- dataSurfaceShmem.mShmem = aBufferHandle.Map();
- if (!dataSurfaceShmem.mShmem) {
- // Try clearing out old mappings to see if resource limits were reached.
- DataSurfaceBufferWillChange(0, false);
- // Try mapping one last time.
- dataSurfaceShmem.mShmem = aBufferHandle.Map();
- if (!dataSurfaceShmem.mShmem) {
- return false;
- }
- }
- }
-
return TranslateRecording();
}
-void CanvasTranslator::GetDataSurface(uint32_t aId, uint64_t aSurfaceRef) {
+void CanvasTranslator::GetDataSurface(uint64_t aSurfaceRef) {
MOZ_ASSERT(IsInTaskQueue());
ReferencePtr surfaceRef = reinterpret_cast<void*>(aSurfaceRef);
@@ -447,31 +362,17 @@ void CanvasTranslator::GetDataSurface(uint32_t aId, uint64_t aSurfaceRef) {
ImageDataSerializer::ComputeRGBStride(format, dstSize.width);
auto requiredSize =
ImageDataSerializer::ComputeRGBBufferSize(dstSize, format);
- if (requiredSize <= 0) {
+ if (requiredSize <= 0 || size_t(requiredSize) > mDataSurfaceShmem.Size()) {
return;
}
// Ensure any old references to the shmem are copied before modification.
- DataSurfaceBufferWillChange(aId);
-
- // Ensure a shmem exists with the given id.
- auto it = mDataSurfaceShmems.find(aId);
- if (it == mDataSurfaceShmems.end()) {
- return;
- }
+ DataSurfaceBufferWillChange();
// Try directly reading the data surface into shmem to avoid further copies.
- if (size_t(requiredSize) > it->second.mShmem.Size()) {
- return;
- }
-
- uint8_t* dst = it->second.mShmem.DataAs<uint8_t>();
+ uint8_t* dst = mDataSurfaceShmem.DataAs<uint8_t>();
if (dataSurface->ReadDataInto(dst, dstStride)) {
- // If reading directly into the shmem, then mark the data surface as the
- // shmem's owner.
- it->second.mOwner = dataSurface;
- dataSurface->AddUserData(&mDataSurfaceShmemIdKey,
- reinterpret_cast<void*>(aId), nullptr);
+ mDataSurfaceShmemOwner = dataSurface;
return;
}
@@ -883,8 +784,8 @@ void CanvasTranslator::HandleCanvasTranslatorEvents() {
dispatchTranslate = AddBuffer(event->TakeBufferHandle());
break;
case CanvasTranslatorEvent::Tag::SetDataSurfaceBuffer:
- dispatchTranslate = SetDataSurfaceBuffer(
- event->mId, event->TakeDataSurfaceBufferHandle());
+ dispatchTranslate =
+ SetDataSurfaceBuffer(event->TakeDataSurfaceBufferHandle());
break;
case CanvasTranslatorEvent::Tag::ClearCachedResources:
ClearCachedResources();
@@ -1481,7 +1382,7 @@ bool CanvasTranslator::PushRemoteTexture(
void CanvasTranslator::ClearTextureInfo() {
MOZ_ASSERT(mIPDLClosed);
- DataSurfaceBufferWillChange(0, false);
+ DataSurfaceBufferWillChange();
mUsedDataSurfaceForSurfaceDescriptor = nullptr;
mUsedWrapperForSurfaceDescriptor = nullptr;
@@ -1850,20 +1751,7 @@ void CanvasTranslator::AddDataSurface(
}
void CanvasTranslator::RemoveDataSurface(gfx::ReferencePtr aRefPtr) {
- RefPtr<gfx::DataSourceSurface> surface;
- if (mDataSurfaces.Remove(aRefPtr, getter_AddRefs(surface))) {
- // If the data surface is the assigned owner of a shmem, and if the shmem
- // is not the last shmem id used, then erase the shmem.
- if (auto id = reinterpret_cast<uintptr_t>(
- surface->GetUserData(&mDataSurfaceShmemIdKey))) {
- if (id != mLastDataSurfaceShmemId) {
- auto it = mDataSurfaceShmems.find(id);
- if (it != mDataSurfaceShmems.end()) {
- mDataSurfaceShmems.erase(it);
- }
- }
- }
- }
+ mDataSurfaces.Remove(aRefPtr);
}
} // namespace layers
diff --git a/gfx/layers/ipc/CanvasTranslator.h b/gfx/layers/ipc/CanvasTranslator.h
@@ -8,7 +8,6 @@
#define mozilla_layers_CanvasTranslator_h
#include <deque>
-#include <map>
#include <memory>
#include <unordered_map>
@@ -113,7 +112,7 @@ class CanvasTranslator final : public gfx::InlineTranslator,
* Sets the shared memory to be used for readback.
*/
ipc::IPCResult RecvSetDataSurfaceBuffer(
- uint32_t aId, ipc::MutableSharedMemoryHandle&& aBufferHandle);
+ ipc::MutableSharedMemoryHandle&& aBufferHandle);
ipc::IPCResult RecvClearCachedResources();
@@ -292,7 +291,7 @@ class CanvasTranslator final : public gfx::InlineTranslator,
void NextBuffer();
- void GetDataSurface(uint32_t aId, uint64_t aSurfaceRef);
+ void GetDataSurface(uint64_t aSurfaceRef);
/**
* Wait for a canvas to produce the designated surface. If necessary,
@@ -353,9 +352,8 @@ class CanvasTranslator final : public gfx::InlineTranslator,
MOZ_ASSERT(mTag == Tag::AddBuffer);
}
CanvasTranslatorEvent(const Tag aTag,
- ipc::MutableSharedMemoryHandle&& aBufferHandle,
- uint32_t aId = 0)
- : mTag(aTag), mBufferHandle(std::move(aBufferHandle)), mId(aId) {
+ ipc::MutableSharedMemoryHandle&& aBufferHandle)
+ : mTag(aTag), mBufferHandle(std::move(aBufferHandle)) {
MOZ_ASSERT(mTag == Tag::SetDataSurfaceBuffer);
}
@@ -370,9 +368,9 @@ class CanvasTranslator final : public gfx::InlineTranslator,
}
static UniquePtr<CanvasTranslatorEvent> SetDataSurfaceBuffer(
- uint32_t aId, ipc::MutableSharedMemoryHandle&& aBufferHandle) {
+ ipc::MutableSharedMemoryHandle&& aBufferHandle) {
return MakeUnique<CanvasTranslatorEvent>(Tag::SetDataSurfaceBuffer,
- std::move(aBufferHandle), aId);
+ std::move(aBufferHandle));
}
static UniquePtr<CanvasTranslatorEvent> ClearCachedResources() {
@@ -398,8 +396,6 @@ class CanvasTranslator final : public gfx::InlineTranslator,
MOZ_ASSERT_UNREACHABLE("unexpected to be called");
return nullptr;
}
-
- uint32_t mId = 0;
};
/*
@@ -412,11 +408,9 @@ class CanvasTranslator final : public gfx::InlineTranslator,
* @returns true if next HandleCanvasTranslatorEvents() needs to call
* TranslateRecording().
*/
- bool SetDataSurfaceBuffer(uint32_t aId,
- ipc::MutableSharedMemoryHandle&& aBufferHandle);
+ bool SetDataSurfaceBuffer(ipc::MutableSharedMemoryHandle&& aBufferHandle);
- void DataSurfaceBufferWillChange(uint32_t aId = 0, bool aKeepAlive = true,
- size_t aLimit = 0);
+ void DataSurfaceBufferWillChange();
bool ReadNextEvent(EventType& aEventType);
@@ -532,14 +526,8 @@ class CanvasTranslator final : public gfx::InlineTranslator,
std::queue<CanvasShmem> mCanvasShmems;
CanvasShmem mCurrentShmem;
gfx::MemReader mCurrentMemReader{0, 0};
- // Track any data surfaces pointing to a shmem mapping.
- struct DataSurfaceShmem {
- ipc::SharedMemoryMapping mShmem;
- ThreadSafeWeakPtr<gfx::DataSourceSurface> mOwner;
- };
- std::map<uint32_t, DataSurfaceShmem> mDataSurfaceShmems;
- // The last shmem id that was assigned to a mapping.
- uint32_t mLastDataSurfaceShmemId = 0;
+ ipc::SharedMemoryMapping mDataSurfaceShmem;
+ ThreadSafeWeakPtr<gfx::DataSourceSurface> mDataSurfaceShmemOwner;
UniquePtr<CrossProcessSemaphore> mWriterSemaphore;
UniquePtr<CrossProcessSemaphore> mReaderSemaphore;
TextureType mTextureType = TextureType::Unknown;
@@ -587,8 +575,6 @@ class CanvasTranslator final : public gfx::InlineTranslator,
std::deque<UniquePtr<CanvasTranslatorEvent>> mPendingCanvasTranslatorEvents;
std::unordered_map<gfx::ReferencePtr, ExportSurface> mExportSurfaces;
-
- gfx::UserDataKey mDataSurfaceShmemIdKey = {0};
};
} // namespace layers
diff --git a/gfx/layers/ipc/PCanvas.ipdl b/gfx/layers/ipc/PCanvas.ipdl
@@ -55,7 +55,7 @@ parent:
/**
* Sets the shared memory to be used for readback.
*/
- async SetDataSurfaceBuffer(uint32_t aId, MutableSharedMemoryHandle aBufferHandle);
+ async SetDataSurfaceBuffer(MutableSharedMemoryHandle aBufferHandle);
/**
* Notify CanvasTranslator it is about to be minimized.
diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml
@@ -6734,11 +6734,6 @@
value: 128
mirror: always
-- name: gfx.canvas.accelerated.max-data-shmems
- type: RelaxedAtomicUint32
- value: 400
- mirror: always
-
- name: gfx.canvas.accelerated.max-export-surfaces
type: RelaxedAtomicUint32
value: 100
