790640 - add Media Fragments for images (spatial dimensions)

Status: NEW

(Core :: Graphics: ImageLib, enhancement)

Description

[Florian Bender]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20120904124322

Steps to reproduce:

Check: http://jsfiddle.net/Wf2Vj/


Actual results:

Full logo is shown. 


Expected results:

Only top-left part of logo should be shown. 

This should work in CSS (image() function, priority) as well as IMG tags. This feature allows easy Image Sprites. Also, this feature is required for implementing the image() function in CSS. 

See http://www.w3.org/TR/2012/PR-media-frags-20120315/#naming-space and http://www.w3.org/TR/2012/CR-css3-images-20120417/#image-notation .

Comment 1

[Seth Fowler [:seth] [:s2h]]

Taking this one as a subproject of bug 801844.

Comment 2

[Seth Fowler [:seth] [:s2h]]

See also bug 648595, which implements the temporal dimension portion of the Media Fragments URI spec.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 1) Add parser support for spatial media fragments.

Still tracking down a lingering crasher in the imagelib-related code for this bug, but I don't foresee any more changes at the parsing level. Going ahead and requesting review for the parsing code.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 1) Add parser support for spatial media fragments.

Moved the enum out of the class to clean up calling code.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 677575 [details] [diff] [review]
(part 1) Add parser support for spatial media fragments.

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

I'll pass this review over to doublec, as he wrote the code in question.

We'll need some reftests here. The video tests are in layout/reftests/{ogg,webm}-video/, though I presume they're blocked on some other patch actually making use of the parsed values?

Comment 6

[Seth Fowler [:seth] [:s2h]]

Part 1 should not produce any visible change in functionality; it just cleans up the API to support what is needed in part 2. Part 2 (which will hopefully be posted today, as I believe I've finally resolved the remaining issues) will  add support for media fragments for images. There will be reftests for that (which will be part 3, also hopefully coming today). I won't be adding support for spatial media fragments for video as part of this bug; I'll be happy to file a new bug, though, which should depend on this one.

Comment 7

[Seth Fowler [:seth] [:s2h]]

See bug 808170 for support for spatial media fragments in video.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Animation of image media fragments won't be resolved in this bug. (More specifically, it will work for vector images but not raster images.) That will be resolved in bug 808189.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 1) Add parser support for spatial media fragments.

Slightly updated version of part 1.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 2) Add support for spatial media fragments to imagelib.

Ready for review on the imagelib changes.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 3) Tests.

Ready for review on the reftest for spatial media fragments.

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 679822 [details] [diff] [review]
(part 2) Add support for spatial media fragments to imagelib.

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

Seth, Jeff Muizelaar and I talked for a while about this, and we agreed that we want him to create a new Image type that can clip according to the media fragments spec.

::: image/src/VectorImage.cpp
@@ +294,5 @@
>    }
>  
> +  if (mHaveRestrictedRegion) {
> +    if (mRestrictedByPercent)
> +      *aWidth = ceil(*aWidth * (mRestrictedRegion.width / 100.0));

You should explicitly cast this since it'll cause warnings. Also, please {} on if bodies.

@@ +571,5 @@
> +    if (mRestrictedByPercent) {
> +      clip.x = floor(aViewportSize.width * (mRestrictedRegion.x / 100.0));
> +      clip.y = floor(aViewportSize.height * (mRestrictedRegion.y / 100.0));
> +      clip.width = ceil(aViewportSize.width * (mRestrictedRegion.width / 100.0));
> +      clip.height = ceil(aViewportSize.height * (mRestrictedRegion.height / 100.0));

Cast to int to avoid warnings.

::: image/src/imgRequest.cpp
@@ +133,5 @@
>  
>    mProperties = do_CreateInstance("@mozilla.org/properties;1");
>  
> +  aURI->CloneIgnoringRef(getter_AddRefs(mURI));
> +  aCurrentURI->CloneIgnoringRef(getter_AddRefs(mCurrentURI));

Why are you making this change?

::: image/src/imgRequestProxy.cpp
@@ +929,5 @@
> +  , mClipRect(aOther.mClipRect)
> +  , mUsePercentCoords(aOther.mUsePercentCoords)
> +{ }
> +
> +imgRequestProxy* NewClippedProxy(imgRequestProxy* aThis)

this should be static, type on its own line

@@ +943,5 @@
> +}
> +
> +// Returns a clipped version of the provided image. If there's an error,
> +// returns the provided image unchanged.
> +static already_AddRefed<Image> ClippedImage(Image* aImage,

put the return type on its own line

@@ +978,5 @@
> +      if (aUsePercentCoords) {
> +        clip.x = floor(imageWidth * (aClipRect.x / 100.0));
> +        clip.y = floor(imageHeight * (aClipRect.y / 100.0));
> +        clip.width = ceil(imageWidth * (aClipRect.width / 100.0));
> +        clip.height = ceil(imageHeight * (aClipRect.height / 100.0));

You need to cast these to int32_t (or whatever nsIntRect uses) because this is implicitly converting from double.

@@ +987,5 @@
> +                     ? aClipRect.width
> +                     : imageWidth - aClipRect.x;
> +        clip.height = aClipRect.y + aClipRect.height <= imageHeight
> +                      ? aClipRect.height
> +                      : imageHeight - aClipRect.y;

Can you rewrite this to not use the ternary operator (use ifs instead)?

@@ +994,5 @@
> +      // The media fragments spec does not allow the case where the upper left
> +      // corner of the clipping region is outside the image. (Note that the
> +      // coordinates are defined to be nonnegative, so we only need to check
> +      // the case where the coordinates are too large.)
> +      if (clip.x >= imageWidth || clip.y >= imageHeight)

{} around if body

@@ +1001,5 @@
> +        rv = aImage->ExtractFrame(imgIContainer::FRAME_CURRENT, clip,
> +                                  imgIContainer::FLAG_NONE,
> +                                  getter_AddRefs(currentFrame));
> +    } else {
> +      rv = NS_ERROR_FAILURE;

Just initialize rv as NS_ERROR_FAILURE when you create it; that'll leave us without any worry of whether it's initialized in all paths, and will let you remove some if bodies.

@@ +1020,5 @@
> +// state needs to be kept in sync with state on the image, we need to back out
> +// that state before the changeover and replay it afterwards. Modifications to
> +// imgRequestProxyClipped::ChangeOwner() may need to be reflected in
> +// imgRequestProxy::ChangeOwner() as well, since it performs a similar role.
> +nsresult imgRequestProxyClipped::ChangeImage(Image* aNewImage)

type on its own line

@@ +1049,5 @@
> +
> +  return NS_OK;
> +}
> +
> +void imgRequestProxyClipped::OnFrameUpdate(const nsIntRect* aRect)

type on its own line

::: image/src/imgRequestProxy.h
@@ +264,5 @@
> +class imgRequestProxyClipped : public imgRequestProxy
> +{
> +public:
> +  imgRequestProxyClipped(nsIntRect const& aClipRect,
> +                         bool aUsePercentCoords);

This should be an enum instead of a boolean.

Comment 13

[Seth Fowler [:seth] [:s2h]]

The wrapper Image type will be implemented in bug 808189.

Comment 14

[cajbir (:cajbir)]

Comment on attachment 679820 [details] [diff] [review]
(part 1) Add parser support for spatial media fragments.

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

Looks good!

::: content/html/content/src/nsMediaFragmentURIParser.cpp
@@ +350,5 @@
>        nsAutoCString value;
>        NS_UnescapeURL(StringHead(nv, index), esc_Ref | esc_AlwaysCopy, name);
>        NS_UnescapeURL(Substring(nv, index + 1, nv.Length()),
>                       esc_Ref | esc_AlwaysCopy, value);
> +      //nsAutoString a = NS_ConvertUTF8toUTF16(name);

May as well remove this line.

Comment 15

[Seth Fowler [:seth] [:s2h]]

Thanks Chris!

Comment 16

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 1) Add parser support for spatial media fragments.

Rebased against the current mozilla-central.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) Store images only on imgRequestProxy, and not on imgRequest.

First step: since we don't want to cache the wrapped Image objects that are used to implement media fragments, we'll need to store a reference to them (indirectly) with each imgRequestProxy instead of at the imgStatusTracker or imgRequest level. With an approach like that in bug 665707 we could switch to using a new imgRequest every time instead of caching imgRequests, which would make this approach unnecessary, but that's no simple matter.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) Ignore an image's URI ref for caching purposes.

Since the cache is keyed on the URI of the image, we want to ensure that the ref part of the URI is always ignored, or the same base URI with different media fragments will get separate cache entries. We hand over the actual URI, including the ref, to the imgRequestProxy, so that it can wrap the Image object appropriately if a media fragment is present.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) Add ImageWrapper superclass for all Image wrapper classes.

Since we're going to need multiple Image wrapper classes to implement this bug, and there will probably be more in the future, it makes sense to make implementing them as painless as possible by adding a superclass that handles the trivial delegation case.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Whoops, sorry about that. Meant to put the patch that I just uploaded as part 3 in bug 826093.

Comment 21

[Joe Drew (not getting mail)]

Comment on attachment 697222 [details] [diff] [review]
(Part 2) Ignore an image's URI ref for caching purposes.

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

::: image/src/imgLoader.cpp
@@ +712,3 @@
>    nsCOMPtr<nsIURI> uri;
>    aRequest->GetURI(getter_AddRefs(uri));
> +  */

presumably you will want to remove this code.

::: image/src/imgRequest.cpp
@@ +106,5 @@
> +    uri = aURI;
> +  }
> +
> +  return uri.forget();
> +}

Why not do this in imgLoader, rather than having to have GetSpecIgnoringRef() everywhere?

Comment 22

[Joe Drew (not getting mail)]

Comment on attachment 697221 [details] [diff] [review]
(Part 1) Store images only on imgRequestProxy, and not on imgRequest.

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

sadface

::: image/src/ImageFactory.cpp
@@ +135,1 @@
>  

Extra blank line here that should be removed.

::: image/src/imgRequestProxy.cpp
@@ +79,5 @@
> +    // imgRequestProxy has its own copy of the wrapper, but since it contains
> +    // very little state this is not too bad in practice, and it is considerably
> +    // better for the common use case of sprite sheets than caching a new copy
> +    // of the entire sheet each time a sprite is used.
> +    if (!!mImage)

may as well just leave this as if (mImage) since we're not assigning to a boolean.

Comment 23

[Joe Drew (not getting mail)]

Comment on attachment 697222 [details] [diff] [review]
(Part 2) Ignore an image's URI ref for caching purposes.

Seth just explained this to me - given that we are basically aliasing images by caching ignoring the ref, but exposing different views to consumers based on their imgRequestProxy (i.e., the exact URI, including the media fragment ref), we can't simply strip out the ref from imgLoader. Sad-face.

Comment 24

[Seth Fowler [:seth] [:s2h]]

Heh, mid-air collision. Indeed, that's the idea. Thanks for the review Joe! I'll get that commented code removed shortly.

Comment 25

[Wesley Johnston (:wesj)]

What do we need to do to land this?

Comment 26

[Seth Fowler [:seth] [:s2h]]

Not that much more, now. This got delayed a lot by issues in patches it depended on - there were SVG-related issues like bug 704059, which are _hopefully_ finally resolved as of yesterday, and there was a memory leak that needed to be tracked down. At this point I mostly need to rebase my implementation against the current version of the code, as quite a bit has changed. Later today I'll update this bug with the remaining dependencies so you can see what's left to do.

Comment 27

[Seth Fowler [:seth] [:s2h]]

So actually the current dependencies are still correct; what we're waiting on here is bug 826093. When that bug is done, the media fragments code will be a pretty simple patch on top of that. Bug 826093 is probably going to end up being divided into three separate bugs; there's one bug already posted there now and currently awaiting review, and two others that will come soon.

Comment 28

[Florian Bender]

All blockers are fixed, can this continue now, please?

Comment 29

[Anne (:annevk)]

How does this feature interact with XMLHttpRequest (bytes returned) or <canvas>.drawImage() (copied)?

Basically, does this happen at the fetch level, decoding level, elsewhere? http://www.w3.org/TR/media-frags/ doesn't seem exactly clear on this.

Comment 30

[Seth Fowler [:seth] [:s2h]]

(In reply to Florian Bender from comment #28)
> All blockers are fixed, can this continue now, please?

Absolutely. Resuming work on this now.

Comment 31

[Seth Fowler [:seth] [:s2h]]

(In reply to Anne (:annevk) from comment #29)
> How does this feature interact with XMLHttpRequest (bytes returned) or
> <canvas>.drawImage() (copied)?
> 
> Basically, does this happen at the fetch level, decoding level, elsewhere?
> http://www.w3.org/TR/media-frags/ doesn't seem exactly clear on this.

If the spec's not clear enough it may be that we need to add some clarification. As currently implemented, this happens more or less at the decode level. Every instance of the same image on a page shares the same underlying network request, regardless of what the media fragment on each instance is. (Well, modulo caching policy and such - basically, two '<img src="foo.jpg">' on the same page are no different from one '<img src="foo.jpg">' and one '<img src="foo.jpg#xywh=...">' for these purposes.)

What this means is that XMLHttpRequest for "foo.jpg#xywh=..." will yield exactly the same bytes as "foo.jpg". As an uninterpreted stream of bytes, the two are the same.

However, anything that interprets the image as an _image_ will see the clipped version of the image. This means that drawImage() on a canvas will draw the clipped version of the image. There will be no exceptions to this. Once the media-fragment-bearing image is "decoded" by imagelib, the effect is the same as if the original image had been clipped in the way specified by the media fragment.

Comment 32

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) Store images only on imgRequestProxy, and not on imgRequest.

Rebase of part 1.

Comment 33

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) Ignore an image's URI ref for caching purposes.

Rebase of part 2.

Comment 34

[Seth Fowler [:seth] [:s2h]]

Since these patches have been gathering dust for a while let's make sure they're still green on try:

https://tbpl.mozilla.org/?tree=Try&rev=b16d49bbd129

Comment 35

[jsnkuhn]

Would love to see this resurrected. It's a much better over all solution for image spiriting. Issues with the current process:

• requires the use of CSS background images even when you shouldn't need them
• syntax is bloated at best, uses up background-size and background-position therefore these properties can't be used for their normal purposes
• background repeat-x or repeat-y requires a specifically designed image
• can't repeat in both directions
• can't use for multiple border-image (requires distance from the edge)

Media Fragments for images would, I believe, fix all of this.

Comment 36

[jsnkuhn]

Very confused by this bug. It looks like a ton of work was put in toward image-fragments but nothing ever came of it?? Or did this lead to -moz-image-rect

Comment 37

[cajbir (:cajbir)]

The Media Fragments proposal didn't carry forward and other browsers weren't keen to implement IIRC.

Comment 38

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.