Closed Bug 959123 Opened 6 years ago Closed 6 years ago

Implement CairoImage::GetAsSourceSurface

Categories

(Core :: Graphics: Layers, defect)

28 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: ali, Assigned: ali)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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
Blocks: 958489
Assignee: nobody → ali
Blocks: 947194
No longer blocks: 958489
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 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+
Blocks: 960053
No longer blocks: 947194
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
https://hg.mozilla.org/mozilla-central/rev/8c9e9d8c4c75
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.
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.
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.
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 :)
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.
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.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.