Closed
Bug 959123
Opened 10 years ago
Closed 10 years ago
Implement CairoImage::GetAsSourceSurface
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: ali, Assigned: ali)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
17.08 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36 Steps to reproduce: Implement CairoImage::GetAsSourceSurface to do what CairoImage::DeprecatedGetAsSurface does for a SourceSurface
Updated•10 years ago
|
Assignee: nobody → ali
Deprecate CairoImage::Data::mSurface and add an mSourceSurface. Also change all the callers of CairoImage::SetData to set the SourceSurface as well. Also change scope of CairoImage::mSource to private
Attachment #8359689 -
Flags: review?(nical.bugzilla)
Comment 3•10 years ago
|
||
Comment on attachment 8359689 [details] [diff] [review] Implement CairoImage::GetAsSourceSurface Review of attachment 8359689 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ImageContainer.h @@ +927,2 @@ > gfx::IntSize mSize; > + RefPtr<gfx::SourceSurface> mSourceSurface; Please add a comment saying that mSourceSurface is wrapping mDeprecatedSurface's data and that therefore it should not outlive mDeprecatedSurface. @@ +961,2 @@ > gfx::IntSize mSize; > + nsCountedRef<nsMainThreadSourceSurfaceRef> mSourceSurface; same comment about the lifetime of mSourceSurface with respect to the one of mDeprecatedSourceSurface.
Attachment #8359689 -
Flags: review?(nical.bugzilla) → review+
Deprecate CairoImage::Data::mSurface and add an mSourceSurface. Also change all the callers of CairoImage::SetData to set the SourceSurface as well. Also change scope of CairoImage::mSource to private
Attachment #8359689 -
Attachment is obsolete: true
Keywords: checkin-needed
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9e9d8c4c75
Keywords: checkin-needed
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c9e9d8c4c75
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 7•10 years ago
|
||
This caused a major performance regression in Windows DXVA accelerated video decoding: Bug 962288 We'll need that fixed quickly, or unfortunately this will need to be backed out.
Comment 8•10 years ago
|
||
I think this is because of the change to ImageLayerD3D10::RenderLayer. You're now calling image->DeprecatedGetAsSurface(); on all images, even if the result won't be needed or used. If image is a Windows DXVA image (D3D9SurfaceImage), then this call triggers very slow readback from the GPU. This is entirely wasted work since we don't use the result, but it slows us down enough to start missing frames.
It is weird because that path didn't chance(In reply to Matt Woodrow (:mattwoodrow) from comment #8) > I think this is because of the change to ImageLayerD3D10::RenderLayer. > > You're now calling image->DeprecatedGetAsSurface(); on all images, even if > the result won't be needed or used. But that was there before as well right. So then this change set would not have been the cause. The only reason I can see if it was this patch causing the regressions is that this patch calls gfxPlatform::GetSourceSurfaceForSurface on the CairoImage::Data::mDeprecatedSurface and assigns the result to CairoImage::Data::mSourceSurface (but that should just wrap gfxASurface??). Otherwise the rest of the changes in the attached patch don't functionally alter anything.
Comment 10•10 years ago
|
||
I don't see that. The call to DeprecatedGetAsSurface() on line 216 of ImageLayerD3D10.cpp looks to be entirely new in this patch. Previously we used static_cast<CairoImage*>, and only if it was CAIRO_SURFACE typed.
Assignee | ||
Comment 11•10 years ago
|
||
Oh wait, it also adds a call to DeprecatedGetAsSurface in ImageLayerD3D10::GetImageSRView. It used to be if (!cairoImage->mSurface) and now it's if (!cairoImage->DeprecatedGetAsSurface()) Anyway, I'll double check if it's those lines when I get in to work in a few hours :)
Assignee | ||
Comment 12•10 years ago
|
||
Ahh(In reply to Matt Woodrow (:mattwoodrow) from comment #10) > I don't see that. The call to DeprecatedGetAsSurface() on line 216 of > ImageLayerD3D10.cpp looks to be entirely new in this patch. > > Previously we used static_cast<CairoImage*>, and only if it was > CAIRO_SURFACE typed. Ahha, yeah you're right! I'll see if changing that fixes things.
Comment 13•10 years ago
|
||
The GetImageSRView one shouldn't matter much, since it's within a CAIRO_SURFACE specific branch. Calling GetAsSurface() on a CarioImage should be the same as casting to a CairoImage* and retrieving mSurface. It's only for the other Image* subclasses where GetAsSurface() might be a much more expensive operation.
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•