Closed Bug 846132 Opened 7 years ago Closed 7 years ago

Remove imgIContainer::CopyFrame

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(3 files, 8 obsolete files)

7.44 KB, patch
Details | Diff | Splinter Review
15.12 KB, patch
Details | Diff | Splinter Review
2.66 KB, patch
Details | Diff | Splinter Review
We don't actually need imgIContainer::CopyFrame. There are too many ways to get access to frames on imgIContainer as it is, and it's desirable to reduce the surface area of the API.

GetFrame can be used for the same things as CopyFrame. The difference, clearly enough, is that GetFrame doesn't necessarily copy the underlying surface (though it can). In situations where GetFrame doesn't copy it can end up returning an optimized surface that can't be read from or written to. We can handle this by adding some convenience methods to gfxASurface - one for the case where we only want to read the resulting surface (which won't necessarily copy) and one for the case where we want to write to it (which always will). Then callers can just call one of these methods on the result of GetFrame, and we've recovered all of the functionality that CopyFrame offered.

This bug is about implementing this change.
Here we add the gfxASurface methods that will eventually replace the functionality of CopyFrame.

(Josh, I figured it'd be good to spread the reviews out a little more, so I'm sending you this one. Let me know if you can't review it.)

One thing I don't like about this patch is that GetAsReadableImageSurface can't return an nsRefPtr<const gfxImageSurface>. I'd love to fix our smart pointer implementation to allow this, but that'll have to happen in a separate bug.
Attachment #719289 - Flags: review?(josh)
Switching CopyFrame callers over to the new methods.
Attachment #719290 - Flags: review?(josh)
Remove CopyFrame and update imgIContainer.
Attachment #719291 - Flags: review?(joe)
Comment on attachment 719289 [details] [diff] [review]
(Part 1) Add gfxASurface methods to produce gfxImageSurfaces.

Review of attachment 719289 [details] [diff] [review]:
-----------------------------------------------------------------

I've got little experience with gfxASurface, so I'm not a good choice here.
Attachment #719289 - Flags: review?(josh)
Comment on attachment 719290 [details] [diff] [review]
(Part 2) Update CopyFrame callers to use GetFrame.

Review of attachment 719290 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsClipboard.mm
@@ +431,5 @@
>          continue;
>        }
>  
> +      nsRefPtr<gfxASurface> frame;
> +      rv = image->GetFrame(   imgIContainer::FRAME_CURRENT,

Let's scrap this weird spacing.

::: widget/cocoa/nsCocoaUtils.mm
@@ +315,5 @@
>  
>  nsresult nsCocoaUtils::CreateNSImageFromImageContainer(imgIContainer *aImage, uint32_t aWhichFrame, NSImage **aResult)
>  {
> +  nsRefPtr<gfxASurface> frame;
> +  nsresult rv = aImage->GetFrame( aWhichFrame,

Here as well.

::: widget/cocoa/nsMenuItemIconX.mm
@@ +388,5 @@
>      mImageRegionRect.SetRect(0, 0, origWidth, origHeight);
>    }
>    
> +  nsRefPtr<gfxASurface> frame;
> +  nsresult rv = imageContainer->GetFrame(   imgIContainer::FRAME_CURRENT,

And here.
Attachment #719290 - Flags: review?(josh) → review+
Comment on attachment 719291 [details] [diff] [review]
(Part 3) - Remove imgIContainer::CopyFrame.

Review of attachment 719291 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/RasterImage.cpp
@@ +988,5 @@
>  
>  
>  //******************************************************************************
>  /* [noscript] gfxImageSurface copyFrame(in uint32_t aWhichFrame,
>   *                                      in uint32_t aFlags); */

Remove this annotation too
Attachment #719291 - Flags: review?(joe) → review+
Comment on attachment 719289 [details] [diff] [review]
(Part 1) Add gfxASurface methods to produce gfxImageSurfaces.

Requesting review for part 1 from Jeff since it sounds like he's the right person for this.
Attachment #719289 - Flags: review?(jmuizelaar)
Updated patch based on review comments. Also, made the same changes to the Windows, Linux, and OS/2 platform-specific code. I forgot to update the patch with those changes before asking for review. It's probably not worth reviewing again since it's a pretty mechanical change that is done in the same way everywhere, so I'll leave the option open but won't set r?.
Attachment #719290 - Attachment is obsolete: true
Updated patch based on review comments.
Attachment #719291 - Attachment is obsolete: true
Thanks for the reviews, Joe and Josh. I've submitted a new try job here:

https://tbpl.mozilla.org/?tree=Try&rev=6158febdde50
Comment on attachment 719289 [details] [diff] [review]
(Part 1) Add gfxASurface methods to produce gfxImageSurfaces.

Review of attachment 719289 [details] [diff] [review]:
-----------------------------------------------------------------

You have r=jdm instead of jrmuizel

::: gfx/thebes/gfxASurface.cpp
@@ +330,5 @@
> +    nsRefPtr<gfxPattern> pattern = new gfxPattern(this);
> +
> +    const gfxIntSize size = GetSize();
> +    nsRefPtr<gfxImageSurface> imgSurface =
> +        new gfxImageSurface(size, gfxASurface::ImageFormatARGB32);

Should this always be ImageFormatARGB32 or should it depend on the format of the image?

@@ +335,5 @@
> +
> +    gfxContext ctx(imgSurface);
> +    ctx.SetOperator(gfxContext::OPERATOR_SOURCE);
> +    ctx.Rectangle(gfxRect(0, 0, size.width, size.height));
> +    ctx.SetPattern(pattern);

you can just use SetSource() instead of creating a pattern.
Attachment #719289 - Flags: review?(jmuizelaar) → review-
Thanks for the review, Jeff. I'll post an updated patch shortly.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #12)
> Should this always be ImageFormatARGB32 or should it depend on the format of
> the image?

We discussed this on IRC. The original CopyFrame code always returns a surface with ImageFormatARGB32. To avoid triggering any issues with callers, we'll address this by keeping this behavior and changing the method name to make the kind of surface you'll receive more explicit.
Here's an updated patch based upon your review comments. The other patches need to be rebased against this one, but I'll hold off until this gets an r+.
Attachment #719722 - Flags: review?(jmuizelaar)
Attachment #719289 - Attachment is obsolete: true
Depends on: 846937
Fix a typo that broke the build on Windows.
Attachment #719689 - Attachment is obsolete: true
Rebased, but no other changes.
Attachment #719692 - Attachment is obsolete: true
OK, here's a new try job rebased on top of bug 846937 (without that we got xpcshell test failures on OS X) and with the Windows-breaking typo fixed. All known problems that I can test locally should be fixed.

https://tbpl.mozilla.org/?tree=Try&rev=b4d801515719
Attachment #719722 - Flags: review?(jmuizelaar) → review+
Thanks for the review, Jeff.

That try job revealed another typo affecting Windows, which this updated patch fixes. It also got polluted by the xpcshell test bustage from Friday. I'll push an updated try job in just a moment.
Attachment #720178 - Attachment is obsolete: true
Argh. OK, that try job looks fine on every platform except Windows, where yet another typo got uncovered. I really wish more than one error got reported at a time. At any rate, here's a new Windows-only try job:

https://tbpl.mozilla.org/?tree=Try&rev=b57ffa0fa76f
Attachment #720945 - Attachment is obsolete: true
Argh, now there are linking issues. I discussed this problem with roc who suggesting trying to make GetAsReadableARGB32ImageSurface virtual to break the link-time dependency between nsWindowsShellService.cpp and gfxASurface.cpp.

Try job here (Windows only as before; all the other platforms look good): https://tbpl.mozilla.org/?tree=Try&rev=02a389af3faf
Attachment #719722 - Attachment is obsolete: true
Depends on: 848841
You need to log in before you can comment on or make changes to this bug.