Open Bug 790640 Opened 11 years ago Updated 3 months ago

add Media Fragments for images (spatial dimensions)


(Core :: Graphics: ImageLib, enhancement)






(Reporter: fb+mozdev, Unassigned)


(Blocks 4 open bugs, )


(Keywords: dev-doc-needed, feature)


(2 files, 9 obsolete files)

10.88 KB, patch
Details | Diff | Splinter Review
19.14 KB, patch
Details | Diff | Splinter Review
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:


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 and .
OS: Mac OS X → All
Hardware: x86 → All
Severity: normal → enhancement
Ever confirmed: true
Version: 16 Branch → Trunk
Assignee: nobody → seth
Component: Layout: Images → ImageLib
Target Milestone: --- → mozilla19
Taking this one as a subproject of bug 801844.
See also bug 648595, which implements the temporal dimension portion of the Media Fragments URI spec.
Blocks: 801844
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.
Attachment #677225 - Flags: review?(cpearce)
Depends on: 807556
Moved the enum out of the class to clean up calling code.
Attachment #677225 - Attachment is obsolete: true
Attachment #677225 - Flags: review?(cpearce)
Attachment #677575 - Flags: review?(cpearce)
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?
Attachment #677575 - Flags: review?(cpearce) → review?(chris.double)
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.
Blocks: 808170
See bug 808170 for support for spatial media fragments in video.
Blocks: 808189
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.
Blocks: 703217
Depends on: 809181
Slightly updated version of part 1.
Attachment #677575 - Attachment is obsolete: true
Attachment #677575 - Flags: review?(chris.double)
Attachment #679820 - Flags: review?(chris.double)
Ready for review on the imagelib changes.
Attachment #679822 - Flags: review?(joe)
Attached patch (part 3) Tests. (obsolete) — Splinter Review
Ready for review on the reftest for spatial media fragments.
Attachment #679824 - Flags: review?(joe)
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(";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.
Attachment #679822 - Flags: review?(joe)
Attachment #679824 - Flags: review?(joe) → review+
No longer blocks: 808189
Depends on: 808189
The wrapper Image type will be implemented in bug 808189.
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.
Attachment #679820 - Flags: review?(chris.double) → review+
Thanks Chris!
Blocks: 419588
Rebased against the current mozilla-central.
Attachment #679820 - Attachment is obsolete: true
Blocks: 821571
Blocks: 823294
Attachment #688046 - Attachment is obsolete: true
Attachment #679822 - Attachment is obsolete: true
Attachment #679824 - Attachment is obsolete: true
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.
Attachment #697221 - Flags: review?(joe)
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.
Attachment #697222 - Flags: review?(joe)
No longer depends on: 808189
Blocks: 808189
Depends on: 825720
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.
Attachment #697226 - Flags: review?(joe)
Attachment #697226 - Attachment is obsolete: true
Attachment #697226 - Flags: review?(joe)
Depends on: 826093
Whoops, sorry about that. Meant to put the patch that I just uploaded as part 3 in bug 826093.
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?
Attachment #697222 - Flags: review?(joe) → review-
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]:


::: 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.
Attachment #697221 - Flags: review?(joe) → review+
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.
Attachment #697222 - Flags: review- → review+
Heh, mid-air collision. Indeed, that's the idea. Thanks for the review Joe! I'll get that commented code removed shortly.
What do we need to do to land this?
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.
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.
No longer blocks: 808189
All blockers are fixed, can this continue now, please?
How does this feature interact with XMLHttpRequest (bytes returned) or <canvas>.drawImage() (copied)?

Basically, does this happen at the fetch level, decoding level, elsewhere? doesn't seem exactly clear on this.
(In reply to Florian Bender from comment #28)
> All blockers are fixed, can this continue now, please?

Absolutely. Resuming work on this now.
(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?
> 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.
Attachment #697221 - Attachment is obsolete: true
Attachment #697222 - Attachment is obsolete: true
Since these patches have been gathering dust for a while let's make sure they're still green on try:
Keywords: feature
Depends on: 940142
Depends on: 977459

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.

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

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

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.

Assignee: seth.bugzilla → nobody
Flags: needinfo?(aosmond)
Severity: normal → S4
Flags: needinfo?(aosmond)
You need to log in before you can comment on or make changes to this bug.