Closed Bug 1256572 Opened 9 years ago Closed 9 years ago

Prevent the memory copy of wrapping DataSourceSurface in off-main-thread painting

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: jerry, Assigned: jerry)

References

Details

(Whiteboard: gfx-noted)

Attachments

(4 files, 25 obsolete files)

P2: use SourceSurfaceRawData with custom deallocator in imgFrame. v4. r=bas
1.84 KB, patch
jerry
: review+
Details | Diff | Splinter Review
P1-1: update DataSourceSurface usage. v5
12.81 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
P0: create DataSourceSurface with current data. v5. r=bas
7.08 KB, patch
jerry
: review+
Details | Diff | Splinter Review
P1-0: setup custom deallocator for SourceSurfaceRawData. v7. r=bas
9.44 KB, patch
jerry
: review+
Details | Diff | Splinter Review
If we try to record the draw commands and replay them at another thread, we might need to copy the DataSourceSurface content into another buffer as [1] during recording. It's because that we can't make sure the DataSourceSurface's life time. This bug will try to check our imageFrame painting flow and have a chance to preserve the image buffer for off-main-thread painting. Then we can reduce the memory copy. [1] https://hg.mozilla.org/mozilla-central/annotate/f0c0480732d36153e8839c7f17394d45f679f87d/gfx/2d/SourceSurfaceRawData.cpp#l30
Blocks: 1256578
Whiteboard: gfx-noted
Assignee: nobody → hshih
Status: NEW → ASSIGNED
Comment on attachment 8731145 [details] [diff] [review] setup the OwnData DataSourceSurface hint in imgFrame. v1 Review of attachment 8731145 [details] [diff] [review]: ----------------------------------------------------------------- I think the image buffer comes from CreateLockedSurface()[1], and that DataSourceSurface already holds the buffer[2](it already setups the userData for release). So, I just pass the ownData hint when we try to create DataSourceSurface in imgFrame. [1] https://hg.mozilla.org/mozilla-central/annotate/5e14887312d4523ab59c3f6c6c94a679cf42b496/image/imgFrame.cpp#l52 [2] https://hg.mozilla.org/mozilla-central/annotate/5e14887312d4523ab59c3f6c6c94a679cf42b496/image/imgFrame.cpp#l68
Attachment #8731145 - Flags: review?(seth)
Attachment #8731145 - Flags: review?(bas)
Comment on attachment 8731145 [details] [diff] [review] setup the OwnData DataSourceSurface hint in imgFrame. v1 Review of attachment 8731145 [details] [diff] [review]: ----------------------------------------------------------------- Unsetting the review flags since the current patch isn't safe. Please rerequest review once you've updated the patch. ::: gfx/2d/Factory.cpp @@ +828,5 @@ > already_AddRefed<DataSourceSurface> > Factory::CreateWrappingDataSourceSurface(uint8_t *aData, int32_t aStride, > const IntSize &aSize, > + SurfaceFormat aFormat, > + bool aOwnData) Please use: |bool aOwnData /* = false */)| ::: image/imgFrame.cpp @@ +63,2 @@ > RefPtr<DataSourceSurface> surf = > + Factory::CreateWrappingDataSourceSurface(*vbufptr, stride, size, format, true); For this to be safe, you need to keep the VolatileBuffer alive. So what I think you need to do is to store a struct in the kVolatileBuffer user data that contains both a RefPtr<VolatileBuffer> and a VolatileBufferPtr, and free the struct in VolatileBufferRelease. Let me know if you need any further explanation.
Attachment #8731145 - Flags: review?(seth)
Attachment #8731145 - Flags: review?(bas)
Comment on attachment 8731145 [details] [diff] [review] setup the OwnData DataSourceSurface hint in imgFrame. v1 Review of attachment 8731145 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/imgFrame.cpp @@ +63,2 @@ > RefPtr<DataSourceSurface> surf = > + Factory::CreateWrappingDataSourceSurface(*vbufptr, stride, size, format, true); The VolatileBuffer is already hold by VolatileBufferPtr[1]. And the VolatileBufferPtr is added into userData. So I think the DataSourceSurface hold both and released with VolatileBufferRelease. [1] https://hg.mozilla.org/mozilla-central/annotate/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/memory/volatile/VolatileBuffer.h#l91
Attachment #8731145 - Flags: feedback?(seth)
update for comment 3.
Attachment #8733239 - Flags: review?(seth)
Attachment #8733239 - Flags: review?(bas)
Attachment #8731145 - Attachment is obsolete: true
Attachment #8731145 - Flags: feedback?(seth)
Comment on attachment 8733239 [details] [diff] [review] setup the OwnData DataSourceSurface hint in imgFrame. v2 Review of attachment 8733239 [details] [diff] [review]: ----------------------------------------------------------------- Request the review again. Please check comment 4. ::: image/imgFrame.cpp @@ +67,5 @@ > return nullptr; > } > > surf->AddUserData(&kVolatileBuffer, vbufptr, VolatileBufferRelease); > + Please check: https://hg.mozilla.org/mozilla-central/annotate/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/memory/volatile/VolatileBuffer.h#l92 and https://hg.mozilla.org/mozilla-central/annotate/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/memory/volatile/VolatileBuffer.h#l108 The VolatileBuffer is held by the VolatileBufferPtr, and the VolatileBufferPtr is held by DataSourceSurface.
Comment on attachment 8733239 [details] [diff] [review] setup the OwnData DataSourceSurface hint in imgFrame. v2 Clear the review. SourceSurfaceRawData's ownData flag doesn't work as I think.
Attachment #8733239 - Flags: review?(seth)
Attachment #8733239 - Flags: review?(bas)
Attachment #8733239 - Attachment is obsolete: true
With SourceSurfaceUserRawData, we can prevent the memory copy in GuaranteePersistance() during painting. The VolatileBuffer is held by the VolatileBufferPtr, and the VolatileBufferPtr is held by this SourceSurfaceUserRawData.
Attachment #8733349 - Flags: review?(seth)
Attachment #8733349 - Flags: review?(bas)
Jerry, sorry for being slow on getting this review done; I'm sick at the moment, and I don't want to review this patch with a head full of cold medicine. =) I'll get it done ASAP, though.
Comment on attachment 8733346 [details] [diff] [review] P1: create a SourceSurfaceUserRawData which could hold the raw buffer. v1 Review of attachment 8733346 [details] [diff] [review]: ----------------------------------------------------------------- SourceSurfaceUserRawData seems to be too close to SourceSurfaceRawData - why not just add a second constructor and extend SourceSurfaceRawData to have a user data type delete callback? Why do we want to duplicate the code? ::: gfx/2d/SourceSurfaceRawData.h @@ +101,5 @@ > + } > + > + virtual ~SourceSurfaceUserRawData() > + { > + MOZ_ASSERT(mMapCount == 0); I don't see how you can assert this. mMapCount depends on the callers "doing the right thing" using public functions. This class has no control over that, and it can't assert it. Same with the mMapCount >= 0 in the Unmap call. The fact that SourceSurfaceRawData does it doesn't make it any better :)
remove InitWrappingData(). Init the members with ctor.
Attachment #8735202 - Flags: review?(bas)
Attachment #8733346 - Attachment is obsolete: true
Attachment #8733346 - Flags: review?(bas)
With UserData, we can prevent the memory copy in GuaranteePersistance() during painting. Seth, you might also reference the P1-1 and P1-2 for the buffer management detail.
Attachment #8735204 - Flags: review?(seth)
Attachment #8735204 - Flags: review?(bas)
Attachment #8733349 - Attachment is obsolete: true
Attachment #8733349 - Flags: review?(seth)
Attachment #8733349 - Flags: review?(bas)
I'll get to this somewhere next week, I'll probably defer part of it to Jeff.
Comment on attachment 8735203 [details] [diff] [review] P1-2: create a new SourceSurfaceRawData ctor with UserData parameters. v1 Review of attachment 8735203 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/SourceSurfaceRawData.h @@ +26,5 @@ > + const IntSize& aSize, > + SurfaceFormat aFormat, > + UserDataKey* aKey, > + void* aUserData, > + void (*aDestroy)(void*)) Boy would this be a lot cleaner if we just used std::function for this kind of task. |aKey|, |aUserData|, and |aDestroy| could all be replaced with one std::function. (Though this patch probably isn't the place to introduce a new convention like that.) @@ +42,1 @@ > // If |aOwnData| is true, the aData will be deleted in dtor. Why do we still need this constructor if we have the new one?
Comment on attachment 8735204 [details] [diff] [review] P2: use SourceSurfaceRawData with UserData in imgFrame. v1 Review of attachment 8735204 [details] [diff] [review]: ----------------------------------------------------------------- Looks safe.
Attachment #8735204 - Flags: review?(seth) → review+
Comment on attachment 8735203 [details] [diff] [review] P1-2: create a new SourceSurfaceRawData ctor with UserData parameters. v1 Review of attachment 8735203 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/SourceSurfaceRawData.h @@ +42,1 @@ > // If |aOwnData| is true, the aData will be deleted in dtor. We still need this. Setup |aOwnData| and |UserData| use different release process in dtor. Please check |~SourceSurfaceRawData()|. If we setup aOwnData, we use delete. If we use UserData, the destroy function will handle this.
Comment on attachment 8735203 [details] [diff] [review] P1-2: create a new SourceSurfaceRawData ctor with UserData parameters. v1 Review of attachment 8735203 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/SourceSurfaceRawData.h @@ +26,5 @@ > + const IntSize& aSize, > + SurfaceFormat aFormat, > + UserDataKey* aKey, > + void* aUserData, > + void (*aDestroy)(void*)) Do we support std::function in gecko? I don't find that in https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #19) > Do we support std::function in gecko? I don't find that in > https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code No, but we have a drop-in replacement in the form of mozilla::Function (defined in mfbt/Function.h).
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #18) > We still need this. Setup |aOwnData| and |UserData| use different release > process in dtor. Please check |~SourceSurfaceRawData()|. > > If we setup aOwnData, we use delete. If we use UserData, the destroy > function will handle this. Yes, but we could just provide a destroy function that deletes the data. Admittedly, because of the API design we'd have to always store the data in a UserData entry.
(In reply to Seth Fowler [:seth] [:s2h] from comment #21) > (In reply to Jerry Shih[:jerry] (UTC+8) from comment #18) > > We still need this. Setup |aOwnData| and |UserData| use different release > > process in dtor. Please check |~SourceSurfaceRawData()|. > > > > If we setup aOwnData, we use delete. If we use UserData, the destroy > > function will handle this. > > Yes, but we could just provide a destroy function that deletes the data. > Admittedly, because of the API design we'd have to always store the data in > a UserData entry. We still need a ctor with |aOwnData==false| that the buffer ownership doesn't transfer to SourceSurfaceRawData. I think the new ctor can't handle that. Or we should pass a non-op function for that? SourceSurfaceRawData(uint8_t* aData, int32_t aStride, const IntSize& aSize, SurfaceFormat aFormat, UserDataKey* aKey, void* aUserData, void (*aDestroy)(void*))
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #22) > (In reply to Seth Fowler [:seth] [:s2h] from comment #21) > > (In reply to Jerry Shih[:jerry] (UTC+8) from comment #18) > > > We still need this. Setup |aOwnData| and |UserData| use different release > > > process in dtor. Please check |~SourceSurfaceRawData()|. > > > > > > If we setup aOwnData, we use delete. If we use UserData, the destroy > > > function will handle this. > > > > Yes, but we could just provide a destroy function that deletes the data. > > Admittedly, because of the API design we'd have to always store the data in > > a UserData entry. > > We still need a ctor with |aOwnData==false| that the buffer ownership > doesn't transfer to SourceSurfaceRawData. > > I think the new ctor can't handle that. Or we should pass a non-op function > for that? That's true. We could either continue to have two constructors or e.g. use a Maybe<Function> argument, with Some() indicating that the data is owned by the SourceSurfaceRawData and hence needs a destructor. However, just to be clear, I think we probably should not be making this change in this patch or even in this bug. We should follow the existing Moz2D style for now, and consider if we want to clean things up in a different bug.
Attachment #8735204 - Flags: review?(bas) → review?(jmuizelaar)
Attachment #8735203 - Flags: review?(bas) → review?(jmuizelaar)
Attachment #8735202 - Flags: review?(bas) → review?(jmuizelaar)
Attachment #8735202 - Flags: review?(jmuizelaar) → review?(bas)
Attachment #8735203 - Flags: review?(jmuizelaar) → review?(bas)
Comment on attachment 8735204 [details] [diff] [review] P2: use SourceSurfaceRawData with UserData in imgFrame. v1 My review queue is too large for me to get to these right now. Bas will be able to get to them sooner.
Attachment #8735204 - Flags: review?(jmuizelaar) → review?(bas)
Comment on attachment 8735202 [details] [diff] [review] P1-1: update SourceSurfaceRawData ctor parameters. v1 Review of attachment 8735202 [details] [diff] [review]: ----------------------------------------------------------------- Moz2D objects are not allowed to be new-ed outside of Moz2D directly. Why can't you use Factory::CreateWrappingDataSourceSurface?
Attachment #8735202 - Flags: review?(bas) → review-
Comment on attachment 8735203 [details] [diff] [review] P1-2: create a new SourceSurfaceRawData ctor with UserData parameters. v1 Review of attachment 8735203 [details] [diff] [review]: ----------------------------------------------------------------- Why do we need this and can't we just call SetUserData? (And you're right, I don't think we can use std::function)
clean up the usage for SourceSurfaceRawData outside of moz2d. Just use Factory::CreateXXXX interface.
Attachment #8742670 - Flags: review?(bas)
Attachment #8735202 - Attachment is obsolete: true
Attachment #8735203 - Attachment is obsolete: true
Attachment #8735203 - Flags: review?(bas)
Comment on attachment 8742671 [details] [diff] [review] P1-2: create a new SourceSurfaceRawData ctor with UserData parameters. v2 Review of attachment 8742671 [details] [diff] [review]: ----------------------------------------------------------------- Why do we need this and can't we just call SetUserData? The are two usage: 1) ownData==true, but no userData We call delete in ctor 2) with userData(it also implies ownData) use userData to delete the buffer I would like to have the constructor to set all flags in the same place. Please check |mOwnData| and |mHasUserData| flag. If we don't create the new ctor, we should add an additional flag in the base class SourceSurface, and set up that flag in function AddUserData(). ::: gfx/2d/SourceSurfaceRawData.h @@ +33,5 @@ > + , mSize(aSize) > + , mFormat(aFormat) > + , mMapCount(0) > + , mOwnData(true) > + , mHasUserData(true) userData also implies ownData. @@ +50,5 @@ > , mSize(aSize) > , mFormat(aFormat) > , mMapCount(0) > , mOwnData(aOwnData) > + , mHasUserData(false) Only setup ownData. No userData here.
Comment on attachment 8742670 [details] [diff] [review] P1-1: update SourceSurfaceRawData ctor parameters. v2 Review of attachment 8742670 [details] [diff] [review]: ----------------------------------------------------------------- This patch clean up the usage of SourceSurface outside moz2d. All SourceSurface will be created from the moz2d factory interface. CreateWrappingDataSourceSurface() has two more flag to fulfill all the callers needs. bool aOwnData = false bool aGuaranteePersistance = false
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #29) > Comment on attachment 8742671 [details] [diff] [review] > P1-2: create a new SourceSurfaceRawData ctor with UserData parameters. v2 > > Review of attachment 8742671 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why do we need this and can't we just call SetUserData? > > The are two usage: > 1) ownData==true, but no userData > We call delete in ctor > 2) with userData(it also implies ownData) > use userData to delete the buffer > > I would like to have the constructor to set all flags in the same place. > Please check |mOwnData| and |mHasUserData| flag. > > If we don't create the new ctor, we should add an additional flag in the > base class SourceSurface, and set up that flag in function AddUserData(). I'm not entirely sure I understand why you need those two things to happen atomically in a single function.
(In reply to Bas Schouten (:bas.schouten) from comment #31) > (In reply to Jerry Shih[:jerry] (UTC+8) from comment #29) > I'm not entirely sure I understand why you need those two things to happen > atomically in a single function. Here are two approach: a) setup |mOwnData| and |mHasUserData| in ctor; Factory::CreateWrappingUserDataSourceSurface(...) { RefPtr<SourceSurfaceRawData> newSurf = new SourceSurfaceRawData(aData, aStride, aSize, aFormat, aKey, aUserData, aDestroy); return newSurf.forget(); } b) set |mHasUserData| in AddUserData(); Factory::CreateWrappingUserDataSourceSurface(...) { RefPtr<SourceSurfaceRawData> newSurf = new SourceSurfaceRawData(aData, aStride, aSize, aFormat, true /* ownData */); newSurf->AddUserData(aKey, aUserData, aDestroy); return newSurf.forget(); } class SourceSurface { .... bool mHasUserData; // this is the hint that the buffer will be released by UserData. } SourceSurface::AddUserData(...) { mHasUserData = true; // setup mHasUserData here. mUserData.Add(key, userData, destroy); } ---- If I use (b), the SourceSurface should add the additional member |mHasUserData|. The |mHasUserData| is only used by SourceSurfaceRawData, so I would like to let SourceSurfaceRawData handle all things. Or we can let AddUserData() become a virtual function and overwrite it in SourceSurfaceRawData.
Flags: needinfo?(bas)
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #32) > (In reply to Bas Schouten (:bas.schouten) from comment #31) > > (In reply to Jerry Shih[:jerry] (UTC+8) from comment #29) > > I'm not entirely sure I understand why you need those two things to happen > > atomically in a single function. > > Here are two approach: > a) setup |mOwnData| and |mHasUserData| in ctor; > Factory::CreateWrappingUserDataSourceSurface(...) > { > RefPtr<SourceSurfaceRawData> newSurf = > new SourceSurfaceRawData(aData, aStride, aSize, aFormat, aKey, > aUserData, aDestroy); > > return newSurf.forget(); > } > > b) set |mHasUserData| in AddUserData(); > Factory::CreateWrappingUserDataSourceSurface(...) > { > RefPtr<SourceSurfaceRawData> newSurf = > new SourceSurfaceRawData(aData, aStride, aSize, aFormat, true /* > ownData */); > > newSurf->AddUserData(aKey, aUserData, aDestroy); > > return newSurf.forget(); > } > > class SourceSurface > { > .... > bool mHasUserData; // this is the hint that the buffer will be released > by UserData. > } > > SourceSurface::AddUserData(...) > { > mHasUserData = true; // setup mHasUserData here. > mUserData.Add(key, userData, destroy); > } > > ---- > If I use (b), the SourceSurface should add the additional member > |mHasUserData|. The |mHasUserData| is only used by SourceSurfaceRawData, so > I would like to let SourceSurfaceRawData handle all things. Or we can let > AddUserData() become a virtual function and overwrite it in > SourceSurfaceRawData. Aside from that HasUserData seems like it would be the same as going a GetUserData and checking if it returns something or not. (mHasUserData would be even more problematic since theoretically it could be totally different user data not actually associated with a destroy function) I think I'm missing some context here, like what are we actually trying to achieve, other than associate a destroy function for deallocation of the data upon the death of the SourceSurface.
Flags: needinfo?(bas)
Like if all we want to do is be able to specify a destroy function, and if that destroy function exists consider the data as belonging to the SourceSurface (i.e. we guarantee lifetime). The right solution here would seem to allow setting a DeallocationFunc on the SourceSurface rather than trying to obscurely hide that fact inside UserData.
update code comment.
Attachment #8744222 - Flags: review?(bas)
Attachment #8742670 - Attachment is obsolete: true
Attachment #8742670 - Flags: review?(bas)
I still setup everything in ctor. If we use SetDeallocationFunc() to set the deallocator, we should handle the problem that we call SetDeallocationFunc() multiple times. We should call the previous deallocator which we set before. And we might also need to update the |mRawData|. So the simplest way is just do everything in ctor.
Attachment #8744230 - Flags: review?(bas)
Attachment #8742671 - Attachment is obsolete: true
Attachment #8742671 - Flags: review?(bas)
update for P1-2 change.
Attachment #8744231 - Flags: review?(bas)
Attachment #8735204 - Attachment is obsolete: true
Attachment #8735204 - Flags: review?(bas)
Comment on attachment 8744222 [details] [diff] [review] P1-1: update SourceSurfaceRawData ctor parameters. v3 Review of attachment 8744222 [details] [diff] [review]: ----------------------------------------------------------------- I don't think we should ever expose GuaranteePersistance to outside Moz2D.
Attachment #8744222 - Flags: review?(bas)
Comment on attachment 8744230 [details] [diff] [review] P1-2: create a new SourceSurfaceRawData ctor with custom deallocator for the surface buffer. v3 Review of attachment 8744230 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/2D.h @@ +1342,5 @@ > + int32_t aStride, > + const IntSize& aSize, > + SurfaceFormat aFormat, > + SourceSurfaceDeallocator aDeallocator, > + void *aBufferData); Call the second void* aClosure, that's more obvious. Also be consistent and use void* not void *.
Attachment #8744230 - Flags: review?(bas) → review+
Attachment #8744231 - Flags: review?(bas) → review+
Attachment #8744222 - Attachment is obsolete: true
Update for Moz2D interface change. We should not use GuaranteePersistance() directly outside Moz2D. If we want to create a data source surface which contains the duplicate data, we should use DataSourceSurface instead of the SourceSurfaceRawData. And do the copy from the original data to DataSourceSurface manually.
Attachment #8744630 - Flags: review?(bas)
Attachment #8744230 - Attachment is obsolete: true
Attachment #8744231 - Attachment is obsolete: true
Comment on attachment 8744629 [details] [diff] [review] P1-0: update SourceSurfaceRawData ctor parameters. v4 Review of attachment 8744629 [details] [diff] [review]: ----------------------------------------------------------------- I'm not 100% sure we actually need to remove this, but it might be better if we just have the one with the destruction functor.
Comment on attachment 8744630 [details] [diff] [review] P1-1: update DataSourceSurface usage. v1 Review of attachment 8744630 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsContentUtils.cpp @@ +7301,5 @@ > > + RefPtr<DataSourceSurface> image = > + Factory::CreateDataSourceSurfaceWithStride(size, > + static_cast<SurfaceFormat>(imageDetails.format()), > + imageDetails.stride()); Shouldn't create it with stride, let the system decide the stride and copy row-by-row. You also may want to consider creating a helper function for this. ::: dom/ipc/TabParent.cpp @@ +1680,5 @@ > + RefPtr<gfx::DataSourceSurface> customCursor = > + gfx::Factory::CreateDataSourceSurfaceWithStride(size, > + static_cast<mozilla::gfx::SurfaceFormat>(aFormat), > + aStride); > + if (customCursor) { idem @@ +3115,5 @@ > mDnDVisualization = > + gfx::Factory::CreateDataSourceSurfaceWithStride(mozilla::gfx::IntSize(aWidth, aHeight), > + static_cast<mozilla::gfx::SurfaceFormat>(aFormat), > + aStride); > + if (mDnDVisualization) { idem
Attachment #8744630 - Flags: review?(bas) → review-
add some functions to create DataSourceSurface from the existing data.
Attachment #8744833 - Flags: review?(bas)
Update for Moz2D interface change.
Attachment #8744836 - Flags: review?(bas)
Attachment #8744630 - Attachment is obsolete: true
fix the already_AddRefed usage.
Attachment #8749183 - Flags: review?(bas)
Attachment #8744833 - Attachment is obsolete: true
Attachment #8744833 - Flags: review?(bas)
update for comment 44. Users could set their custom deallocator for the input aData, and there is also no memory copy in GuaranteePersistance() call. Otherwise, the aData should be released only after destruction of the surface.
Attachment #8749188 - Flags: review?(bas)
Attachment #8744629 - Attachment is obsolete: true
Attachment #8744632 - Attachment is obsolete: true
Attachment #8744629 - Flags: review?(bas)
Attachment #8744836 - Attachment is obsolete: true
Attachment #8744836 - Flags: review?(bas)
Comment on attachment 8749189 [details] [diff] [review] P1-1: update DataSourceSurface usage. v3 Review of attachment 8749189 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLTextureUpload.cpp @@ -182,5 @@ > > const gfx::IntSize size(imageData->Width(), imageData->Height()); > const size_t stride = size.width * 4; > const gfx::SurfaceFormat surfFormat = gfx::SurfaceFormat::R8G8B8A8; > - const bool ownsData = false; This SourceSurfaceRawData doesn't own the buffer. @@ +192,5 @@ > + const RefPtr<gfx::SourceSurface> surf = > + gfx::Factory::CreateWrappingDataSourceSurface(wrappableData, > + stride, > + size, > + surfFormat); Since there is no deallocator, the SourceSurfaceRawData doesn't own the buffer, too. ::: gfx/2d/MacIOSurface.cpp @@ +477,5 @@ > + bytesPerRow, > + IntSize(ioWidth, ioHeight), > + format, > + &MacIOSurfaceBufferDeallocator, > + static_cast<void*>(dataCpy)); The dataCpy is created using malloc so use free in deallocator.
Attachment #8744634 - Attachment is obsolete: true
Comment on attachment 8749183 [details] [diff] [review] P0: create DataSourceSurface with current data. v2 Review of attachment 8749183 [details] [diff] [review]: ----------------------------------------------------------------- This belongs in DataSurfaceHelpers.h/cpp, which already has a CopyRect method you can reuse, fwiw.
Attachment #8749183 - Flags: review?(bas) → review-
Comment on attachment 8749188 [details] [diff] [review] P1-0: create a new SourceSurfaceRawData ctor with custom deallocator for the surface buffer. v5 Review of attachment 8749188 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. But why are you making this a constructor instead of just adding an Init function or adding this as an argument to the init function?
Attachment #8749188 - Flags: review?(bas)
Comment on attachment 8749189 [details] [diff] [review] P1-1: update DataSourceSurface usage. v3 Review of attachment 8749189 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good but needs to be updated to use the proper helper function ::: gfx/2d/MacIOSurface.cpp @@ +477,5 @@ > + bytesPerRow, > + IntSize(ioWidth, ioHeight), > + format, > + &MacIOSurfaceBufferDeallocator, > + static_cast<void*>(dataCpy)); Is there a reason we're using mallow and not new[] here? This is actually currently a bug then, good catch.
Attachment #8749189 - Flags: review?(bas)
Reuse CopyRect() in DataSurfaceHelpers.h
Attachment #8749731 - Flags: review?(bas)
Attachment #8749183 - Attachment is obsolete: true
Attachment #8749188 - Attachment is obsolete: true
Attachment #8749189 - Attachment is obsolete: true
Comment on attachment 8749740 [details] [diff] [review] P1-0: setup custom deallocator for SourceSurfaceRawData. v6 Review of attachment 8749740 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/Factory.cpp @@ +813,5 @@ > { > if (aSize.width <= 0 || aSize.height <= 0) { > return nullptr; > } > + if (!aDeallocator && aClosure) { If we only have aClosure data, return nullptr. It's possible that only provide the deallocator without aClosure. ::: gfx/2d/SourceSurfaceRawData.cpp @@ +25,5 @@ > mStride = aStride; > mFormat = aFormat; > + > + if (aDeallocator) { > + mOwnData = true; Set mOwnData if we have custom deallocator. ::: gfx/2d/SourceSurfaceRawData.h @@ +24,5 @@ > + , mFormat(SurfaceFormat::UNKNOWN) > + , mMapCount(0) > + , mOwnData(false) > + , mDeallocator(nullptr) > + , mClosure(nullptr) init all member to prevent the compiling warnings @@ +80,5 @@ > + friend class Factory; > + > + // If we have a custom deallocator, the |aData| will be released using the > + // custom deallocator and |aClosure| in dtor. > + void InitWrappingData(unsigned char *aData, Make InitWrappingData to be private. So user can't init the SourceSurfaceRawData outside moz2d. @@ +145,5 @@ > bool aZero); > bool InitWithStride(const IntSize &aSize, > SurfaceFormat aFormat, > int32_t aStride, > bool aZero); Make Init and InitWithStride to be private. So user can't init the surface outside moz2d.
Comment on attachment 8749741 [details] [diff] [review] P1-1: update DataSourceSurface usage. v4 Review of attachment 8749741 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLTextureUpload.cpp @@ +190,3 @@ > uint8_t* wrappableData = (uint8_t*)data; > + > + const RefPtr<gfx::SourceSurface> surf = this surface doesn't own the buffer data. ::: gfx/2d/MacIOSurface.cpp @@ +460,5 @@ > size_t ioHeight = GetDevicePixelHeight(); > > unsigned char* ioData = (unsigned char*)GetBaseAddress(); > + unsigned char* dataCpy = > + new unsigned char[bytesPerRow * ioHeight / sizeof(unsigned char)]; turn to use new operator @@ +471,5 @@ > > SurfaceFormat format = HasAlpha() ? mozilla::gfx::SurfaceFormat::B8G8R8A8 : > mozilla::gfx::SurfaceFormat::B8G8R8X8; > > + RefPtr<mozilla::gfx::DataSourceSurface> surf = This surface own the data "dataCpy".
Comment on attachment 8749731 [details] [diff] [review] P0: create DataSourceSurface with current data. v3 Review of attachment 8749731 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/2D.h @@ +1308,5 @@ > + static already_AddRefed<DataSourceSurface> > + CreateDataSourceSurfaceFromData(const IntSize& aSize, > + SurfaceFormat aFormat, > + const uint8_t* aData, > + int32_t aDataStride); Again, these should be separate helpers, not in 2D.h, put them in DataSurfaceHelpers.h
Attachment #8749731 - Flags: review?(bas) → review-
Comment on attachment 8749740 [details] [diff] [review] P1-0: setup custom deallocator for SourceSurfaceRawData. v6 Review of attachment 8749740 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/2D.h @@ +1343,5 @@ > + * > + * We can provide a custom destroying function for |aData|. The |aData| will > + * be released in surface dtor using deallocator and the |aClosure|. If there > + * are some errors during construction(return a nullptr surface), the caller > + * is responsible for the deallocating of the |aClosure| data. Nit, spelling: * We can provide a custom destroying function for |aData|. This will be * called in the surface dtor using |aDeallocator| and the |aClosure|. If there * are errors during construction(return a nullptr surface), the caller is * responsible for the deallocation. @@ +1346,5 @@ > + * are some errors during construction(return a nullptr surface), the caller > + * is responsible for the deallocating of the |aClosure| data. > + * > + * If there is no destroying function, the caller is responsible for > + * deallocating the aData memory only after destruction of the surface. nit: after it releases the last reference to the surface (this may not be when the surface gets destroyed, that's what GuaranteePersistance is force) ::: gfx/2d/SourceSurfaceRawData.h @@ +33,5 @@ > + { > + if (mDeallocator) { > + mDeallocator(mClosure); > + } else if (mOwnData) { > + // The buffer is created from GuaranteePersistance(). nit: This comment isn't accurate, the Surface could also just have been create with CreateDataSourceSurface.
Attachment #8749740 - Flags: review?(bas) → review+
Comment on attachment 8749741 [details] [diff] [review] P1-1: update DataSourceSurface usage. v4 Review of attachment 8749741 [details] [diff] [review]: ----------------------------------------------------------------- This just needs updating for the change in helper functions.
Attachment #8749741 - Flags: review?(bas)
Comment on attachment 8749740 [details] [diff] [review] P1-0: setup custom deallocator for SourceSurfaceRawData. v6 Review of attachment 8749740 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/2D.h @@ +1346,5 @@ > + * are some errors during construction(return a nullptr surface), the caller > + * is responsible for the deallocating of the |aClosure| data. > + * > + * If there is no destroying function, the caller is responsible for > + * deallocating the aData memory only after destruction of the surface. Sorry, I'm not sure what I need to update for this comment. "If there is no destroying function, the caller is responsible for deallocating the aData memory only after destruction of the surface." I think this is enough to show that the buffer needs to be delete after the surface gets destroyed even though we will make a copy in GuaranteePersistance(). We don't have interface to show the ownData status, so always releasing the buffer after the surface gone is safer. ::: gfx/2d/SourceSurfaceRawData.h @@ +33,5 @@ > + { > + if (mDeallocator) { > + mDeallocator(mClosure); > + } else if (mOwnData) { > + // The buffer is created from GuaranteePersistance(). The ownData is only used for SourceSurfaceRawData. And we only set ownData in GuaranteePersistance(). Please check: https://dxr.mozilla.org/mozilla-central/search?q=mOwnData&case=true
Hi Bas, Could you please check the comment 64?
Flags: needinfo?(bas)
Move CreateDataSourceSurfaceFromData into DataSurfaceHelpers.h
Attachment #8750209 - Flags: review?(bas)
Attachment #8749731 - Attachment is obsolete: true
use helper function in DataSurfaceHelpers.h
Attachment #8750210 - Flags: review?(bas)
Attachment #8749741 - Attachment is obsolete: true
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #64) > Comment on attachment 8749740 [details] [diff] [review] > P1-0: setup custom deallocator for SourceSurfaceRawData. v6 > > Review of attachment 8749740 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/2d/2D.h > @@ +1346,5 @@ > > + * are some errors during construction(return a nullptr surface), the caller > > + * is responsible for the deallocating of the |aClosure| data. > > + * > > + * If there is no destroying function, the caller is responsible for > > + * deallocating the aData memory only after destruction of the surface. > > Sorry, I'm not sure what I need to update for this comment. > > "If there is no destroying function, the caller is responsible for > deallocating the aData memory only after destruction of the surface." > I think this is enough to show that the buffer needs to be delete after the > surface gets destroyed even though we will make a copy in > GuaranteePersistance(). We don't have interface to show the ownData status, > so always releasing the buffer after the surface gone is safer. My point is the surface can end up -being- destroyed -after- the caller deallocates the buffer because it's being kept alive by Moz2D. But the caller doesn't need to care about that. > ::: gfx/2d/SourceSurfaceRawData.h > @@ +33,5 @@ > > + { > > + if (mDeallocator) { > > + mDeallocator(mClosure); > > + } else if (mOwnData) { > > + // The buffer is created from GuaranteePersistance(). > > The ownData is only used for SourceSurfaceRawData. > And we only set ownData in GuaranteePersistance(). > Please check: > https://dxr.mozilla.org/mozilla-central/search?q=mOwnData&case=true Ah, you're absolutely right, this code changed. My bad.
Flags: needinfo?(bas)
Comment on attachment 8750209 [details] [diff] [review] P0: create DataSourceSurface with current data. v4 Review of attachment 8750209 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/2D.h @@ +1310,5 @@ > static already_AddRefed<DataSourceSurface> > + CreateDataSourceSurfaceWithStride(const IntSize &aSize, > + SurfaceFormat aFormat, > + int32_t aStride, > + bool aZero = false); nit: Although I probably prefer the changed structure let's not include this hunk to avoid poisoning the blame.
Attachment #8750209 - Flags: review?(bas) → review+
Attachment #8750210 - Flags: review?(bas) → review+
Attachment #8750209 - Attachment is obsolete: true
Attachment #8749740 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: