825771 - [css3-images] implement 'image-orientation' property

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should implement the image-orientation property in:
  http://dev.w3.org/csswg/css3-images/#image-orientation
which allows authors to specify 
This property is in Candidate Recommendation:
  http://www.w3.org/TR/2012/CR-css3-images-20120417/#the-image-orientation

We should additionally implement a from-image value, possibly prefixed, that addresses my comments in:
  http://lists.w3.org/Archives/Public/www-style/2011Dec/0001.html
and allows authors to opt in to honoring EXIF orientation, which is particularly important for Thunderbird (and allows fixing bug 298619).

(I really dislike the way this property is specified, but I lost that argument.)

The Style System side of this is easy; I'm not sure how hard the ImageLib side is, but cc:ing some people who would know.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That third line should have been:

which allows authors to specify a right-angle rotation that can be applied to the image data before any other processing.

Comment 2

[fantasai]

I'm not sure that this is actually a useful thing to implement without the from-image value. It's in the spec because it was needed by the print industry, but is there a need in the Web industry (from-image aside)?

> (I really dislike the way this property is specified, but I lost that argument.)

Great to know, maybe you could elucidate on that point?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to fantasai from comment #2)
> > (I really dislike the way this property is specified, but I lost that argument.)
> 
> Great to know, maybe you could elucidate on that point?

Just what's in the aforementioned:
  http://lists.w3.org/Archives/Public/www-style/2011Dec/0001.html

Comment 4

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

Joe may want to weigh in here too, but FWIW from my perspective this doesn't look too bad now that the refactoring we've done for media fragments is in place.

Comment 5

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

(from an imagelib perspective, that is)

Comment 6

[Joe Drew (not getting mail)]

Presuming that we just add it as another "transformation" (changing size, drawing differently) that your extra Image implementation will be adding, yeah, seems totally straightforward.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://lists.w3.org/Archives/Public/www-style/2011Dec/0001.html
http://lists.w3.org/Archives/Public/www-style/2011Dec/0003.html
describe from-image. Tab's 'flip' proposal, which I think should go along, specifies both order (after the rotation) and axis (inline).  Seth points out the order should probably match the textual order in the style sheet, as Tab does.

I think we should implement that.

Comment 8

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

This shouldn't be too hard if we can get bug 869723 taken care of.

Comment 9

[Fred Wenzel [:wenzel]]

(In reply to Seth Fowler [:seth] from comment #8)
> This shouldn't be too hard if we can get bug 869723 taken care of.

Sorry, I might be dense, but I don't see how bug 869723 blocks this one. From reading the image-orientation spec, there does not seem to be an opt-in to honoring EXIF data. While this CSS property could be used to implement EXIF-based rotation, this property defines orientation manually, and not from metadata. Right?

Comment 10

[Cameron McCormack (:heycam)]

Looks like the "take the orientation from the image" value got pushed to css-images-4:

  http://dev.w3.org/csswg/css-images-4/#the-image-orientation

Comment 11

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

(In reply to Fred Wenzel [:wenzel] from comment #9)
> (In reply to Seth Fowler [:seth] from comment #8)
> > This shouldn't be too hard if we can get bug 869723 taken care of.
> 
> Sorry, I might be dense, but I don't see how bug 869723 blocks this one.
> From reading the image-orientation spec, there does not seem to be an opt-in
> to honoring EXIF data. While this CSS property could be used to implement
> EXIF-based rotation, this property defines orientation manually, and not
> from metadata. Right?

Yes, that's right, but we're not implementing the version in css-images-3. We're implementing Tab's proposal as mentioned in comment 7.

Comment 12

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

(Which is a superset of the version in css-images-3, to be clear.)

Comment 13

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

(In reply to Cameron McCormack (:heycam) from comment #10)
> Looks like the "take the orientation from the image" value got pushed to
> css-images-4:
> 
>   http://dev.w3.org/csswg/css-images-4/#the-image-orientation

That's essentially what we're implementing except that we'll also implement the "flip" modifier mentioned in Issue 18.

Comment 14

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

Attachment: (Part 1) - Add CSS support for the image-orientation property.

Uploading an incomplete patch. This builds but it needs more work - in particular, because image-orientation is an inherited property, I don't see a good existing style struct to place it on. It may need a new one, which perhaps isn't so bad since there are some other properties (image-resolution, image-rendering) that would fit in on the same style struct logically.

Comment 15

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

After talking to dbaron we've agreed that this can go on the visibility or user interface style structs.

Comment 16

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

Attachment: (Part 1) - Add CSS support for the image-orientation property.

This (unfortunately fairly large) patch adds support for the image-orientation property to the CSS system.

Later parts coming in the near future will add support for the property in the layout code and appropriate reftests.

Comment 17

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

Attachment: (Part 2) - Add support for image-orientation to nsImageFrame.

This patch adds image-orientation support to nsImageFrame (in the process modifying it to store a reference directly to the image container it displays, instead of always accessing it through the image request). Since nsImageFrame covers both image elements and generated content, this is enough to handle the important cases described in the spec. (The spec specifically forbids applying image-orientation to 'decoration' images like background-image.)

Per discussion with dbaron, I need to ping the list about whether image-orientation should be supported for list-style-image and within SVGs. I'll create a followup bug for those cases shortly.

Still to come: reftests.

Comment 18

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

Since I've changed some internal details of how nsImageFrame works, I went ahead and pushed a try job for the entire patch stack so far. I'll need another one once the reftests are ready, but I want some early warning if any issues have arisen.

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

Comment 19

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

Attachment: (Part 3) - Add image-orientation reftests.

Here are the first reftests. Not ready for review yet as more are needed; this only covers from-image for raster images.

Tests still needed:
* Explicit orientations for raster images.
* Generated content.
* Test that background-image and border-image don't work.
* Non-axis-aligned angles. (To verify that they round appropriately.)
* Tests for SVG-as-image in <img> tags. (Explicit orientations and a single from-image, since SVG images don't have EXIF.)

Comment 20

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

Interesting. So it looks like those changes to nsImageFrame make a previously intermittent failure happen 100% of the time. That might be good news, actually.

Comment 21

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

Attachment: (Part 2) - Add support for image-orientation to nsImageFrame.

Fixed the reftest failure above. We needed to update nsImageFrame::mImage in NotifyNewCurrentRequest as well as in OnStartRequest, since we do not get the latter in a synchronous manner.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 781495 [details] [diff] [review]
(Part 1) - Add CSS support for the image-orientation property.

Note that nsStyleConsts.h moved from layout/base to layout/style 
a day or two ago.  Patch should apply fine in the new location,
but it needs manual fixup to get there.

nsCSSParser:

>+ * <angle> flip? | flip | from-image
(and the corresponding code)

Why do you accept 'flip' on its own?  (And is it intended that you
accept <angle> flip and not flip <angle>?)

Though given that I'm not seeing 'flip' in the current spec draft, I
wonder if we should accept it at all.  It seems bad not to offer the
same feature set that from-image offers, though.


>+    if (ParseVariant(flip, VARIANT_KEYWORD | VARIANT_STRING,
>+                     nsCSSProps::kImageOrientationFlipKTable)) {

This (and the next) should have VARIANT_KEYWORD only, not VARIANT_STRING.

nsComputedDOMStyle:

>+    nsCSSKeyword keyword =
>+      nsCSSProps::ValueToKeywordEnum(NS_STYLE_IMAGE_ORIENTATION_FROM_IMAGE,
>+          nsCSSProps::kImageOrientationKTable);
>+    string.Append(NS_ConvertUTF8toUTF16(nsCSSKeywords::GetStringValue(keyword)));

string.AppendLiteral("from-image");

>+      string.AppendLiteral(" ");
>+
>+      nsCSSKeyword keyword =
>+        nsCSSProps::ValueToKeywordEnum(NS_STYLE_IMAGE_ORIENTATION_FLIP,
>+            nsCSSProps::kImageOrientationFlipKTable);
>+      string.Append(NS_ConvertUTF8toUTF16(nsCSSKeywords::GetStr

string.AppendLiteral(" from-image");


I'm inclined to say that nsStyleImageOrientation should live in
nsStyleStruct.h rather than nsStyleCoord.h


nsStyleStruct.cpp:

>+    if ((mImageOrientation != aOther.mImageOrientation)) {
>+      NS_UpdateHint(hint, nsChangeHint_NeedReflow);
>+    }

This should be nsChangeHint_AllReflowHints.  It can change intrinsic
widths, and it needs to reflow all descendants, and it needs to mark the
frame dirty.

nsStyleUtil::AppendAngleValue:

This should take the nsStyleCoord by reference rather than by value.

You should just use aResult.AppendFloat rather than constructing a
temporary nsROCSSPrimitiveValue.


r=dbaron with that fixed, although we might need some discussion about the flip questions

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 782781 [details] [diff] [review]
(Part 2) - Add support for image-orientation to nsImageFrame.

Transferring this request to dholbert; sorry, should have done that sooner.

Comment 24

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

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #22)
> Comment on attachment 781495 [details] [diff] [review]
> Why do you accept 'flip' on its own?  (And is it intended that you
> accept <angle> flip and not flip <angle>?)

Tab's proposed grammar was this:
> image-orientation: from-image | [ <angle>? flip? ]

It seemed to me that he was proposing to allow flip by itself. I have no problem with disallowing that, though; it certainly complicates the code a bit.

> Though given that I'm not seeing 'flip' in the current spec draft, I
> wonder if we should accept it at all.  It seems bad not to offer the
> same feature set that from-image offers, though.

IMO we should go ahead and offer it. It seems only logical to reproduce the feature-set of the EXIF orientations. It wouldn't save anybody any code, anyway.

Thanks for the review! Will go ahead and make the rest of the changes tomorrow.

Comment 25

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

Attachment: (Part 3) - Add image-orientation reftests.

Uploading more reftests. We now have reftests for generated content, for non-axis-aligned angles, and for explicit orientations.

Still needed:

* Test that background-image and border-image don't work.
* Tests for SVG-as-image. (As described in comment 19.)

Comment 26

[Daniel Holbert [:dholbert]]

Just a heads-up: I'm on PTO through the weekend (possibly for some of Mon & Tues) -- I'll be traveling to & from Michigan for a friend's wedding.

Hoping to review this on the plane tomorrow, though.

Comment 27

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

Attachment: (Part 2) - Add support for image-orientation to nsImageFrame.

Have fun at the wedding, Daniel!

I've updated the patch slightly to better handle an unlikely failure case in NotifyNewCurrentRequest.

Comment 28

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

Attachment: (Part 3) - Add image-orientation reftests for raster images.

This version has every needed reftest except for those related to SVG-as-image. I've decided to place those in a separate patch so that dholbert can review them, so this patch is now complete and ready for review.

Comment 29

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

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #23)
> Transferring this request to dholbert; sorry, should have done that sooner.

This means I need to update the reviewer on the commit message (mentioning this mainly as a note to myself). I'll wait to do that until Daniel reviews the patch, though, and include that change with the other review changes.

Comment 30

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

Comment on attachment 784550 [details] [diff] [review]
(Part 3) - Add image-orientation reftests for raster images.

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

Switching this over to you, Daniel, per discussion with dbaron.

Comment 31

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

Attachment: (Part 1) - Add CSS support for the image-orientation property.

All of the review comments above are addressed in this version of the patch except for the issue of whether we should accept 'flip' by itself. I think dbaron and I both don't feel strongly about this, so I'm going to ask on www-style (need to ask about list-style-image anyway) and we can make a final decision after we see if any other stakeholders have opinions on this.

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 784520 [details] [diff] [review]
(Part 2) - Add support for image-orientation to nsImageFrame.

So, the "nsImageFrame keeping a persistent handle to its imgIContainer" part of this patch is a bit of a change in code-responsibility.  It seems reasonable to me, but I think it'd be good to have joe or an imagelib peer sign off on the idea, at least (maybe someone already has?), since there are surely (historical?) reasons that we go through the hoops we currently go through, everywhere in nsImageFrame where we need the imgIContainer.

I'll proceed under the assumption that this has imagelib approval for now, anyway, and it looks like you're probably keeping mImage appropriately up-to-date.

I also think it'd be worth splitting this patch into two atomic parts:

 - PART A: Refactor nsImageFrame to use "mImage".
   (i.e. the existing patch's nsImageFrame edits, minus the orientation stuff.)
   (This patch hopefully shouldn't have any functional effect.)

 - PART B: Add nsLayoutUtils::OrientImage(), and invoke it from nsImageFrame where appropriate.

I'll review it all together for now - but I do think it'd be worth splitting up like that before you land, to facilitate regression-hunting and to improve changeset-understandability for future mozilla-central archeologists who are trying to grok what's changing here.

>+    Angle angle;
>+
>+    if      (aOrientation.Is0Degrees())   angle = Angle::D0;
>+    else if (aOrientation.Is90Degrees())  angle = Angle::D90;
>+    else if (aOrientation.Is180Degrees()) angle = Angle::D180;
>+    else if (aOrientation.Is270Degrees()) angle = Angle::D270;
>+    else                                  MOZ_ASSERT(false, "Invalid orientation");

As elegant as this block looks, it's not very debuggable/breakpointable, and it doesn't really match mozilla/local style. I'd marginally prefer the more mozilla-coding-style-conforming multi-line braced form -- but I do see you did something similar in the other patch here, and it looks like dbaron didn't object in that instance, so I won't object too strongly here, if you want to keep it.

However: in the final "else" clause, you're definitely going to need to add curly-braces, because you need to add another line of code there. You need to either initialize |angle| there, or to do an early return. Otherwise, if we take that "else", we'll end up using |angle| without ever initializing it (which Clang will probably spam a compiler warning about, "-Wsometimes-initialized" or something like that). (Alternately, you could just give |angle| a default value up top - but that's probably hackier & a bit wasteful (double-setting it in all the normally-traversed cases)).

>diff --git a/layout/base/nsLayoutUtils.h b/layout/base/nsLayoutUtils.h
>+  static already_AddRefed<imgIContainer> OrientImage(imgIContainer* aContainer,
>+                                                     const nsStyleImageOrientation& aOrientation);

Add a newline before "OrientImage" so that these lines aren't so huge.
(To match local style of e.g. AssertNoDuplicateContinuations, a bit further down, you'll want to align "OrientImage" right underneath "static" -- no need for bonus indentation.)

>diff --git a/layout/generic/nsImageFrame.cpp b/layout/generic/nsImageFrame.cpp
> nsImageFrame::NotifyNewCurrentRequest(imgIRequest *aRequest,
>                                       nsresult aStatus)
> {
>+  nsCOMPtr<imgIContainer> image;
>+  aRequest->GetImage(getter_AddRefs(image));
>+  NS_ASSERTION(mImage || NS_FAILED(aStatus), "Successful load with no container?");
>+
[...]
>-  if (NS_SUCCEEDED(aStatus)) {
>+  if (NS_SUCCEEDED(aStatus) && mImage) {
>+    mImage = nsLayoutUtils::OrientImage(image, StyleVisibility()->mImageOrientation);

I'm not clear on why we need to add the |mImage| null-check here, when we're about to clear out its existing value anyway. Maybe add a comment to clarify that, if the check is needed?  Or convert it to an assertion (if aStatus is success), if that makes sense?

Also: move the |image| local-var (and the GetImage() call that populates it) down inside the "if"-guarded code, since that's the only place you use it, and also to match the old code's "check aStatus before you call GetImage" ordering.

> nsImageFrame::EnsureIntrinsicSizeAndRatio(nsPresContext* aPresContext)
> {
>-    // Jump through all the hoops to get the status of the request
>-    nsCOMPtr<imgIRequest> currentRequest;
>-    nsCOMPtr<nsIImageLoadingContent> imageLoader = do_QueryInterface(mContent);
>-    if (imageLoader)
>-      imageLoader->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST,
>-                              getter_AddRefs(currentRequest));
>-    uint32_t status = 0;
>-    if (currentRequest)
>-      currentRequest->GetImageStatus(&status);
>-
>-    // If we know the size, we can grab it and use it for an update
>-    if (status & imgIRequest::STATUS_SIZE_AVAILABLE) {
>-      nsCOMPtr<imgIContainer> imgCon;
>-      currentRequest->GetImage(getter_AddRefs(imgCon));
>-      NS_ABORT_IF_FALSE(imgCon, "SIZE_AVAILABLE, but no imgContainer?");
>-      UpdateIntrinsicSize(imgCon);
>-      UpdateIntrinsicRatio(imgCon);
>+    if (mImage) {
>+      UpdateIntrinsicSize(mImage);
>+      UpdateIntrinsicRatio(mImage);

Hmm, so this is removing a lot of status-checking. I tend to think we should preserve these checks, for the time being, in an #ifdef DEBUG block inside the "if (mImage)" clause. (with assertions replacing the old code's "if" checks).  That'll help ensure we're not accidentally removing/violating any invariants that we had previously baked in. (e.g. it'll tell us if mImage gets set to something non-null by some odd code-path that doesn't satisfy the other checks that we have here.)

Same goes for DisplayAltFeedback() and BuildDisplayList(), too - the patch removes a lot of status-checking there that could probably be converted into an assertion or two, to make sure we're honoring the existing assumptions.

r=me with the above addressed.

Comment 33

[Daniel Holbert [:dholbert]]

Comment on attachment 784550 [details] [diff] [review]
(Part 3) - Add image-orientation reftests for raster images.

>+# Tests for image-orientation used with 'from-image':
>+fuzzy(2,5) == image-orientation-from-image.html?none     image-orientation-ref.html?0
>+fuzzy(1,1) == image-orientation-from-image.html?0        image-orientation-ref.html?0
>+fuzzy(1,1) == image-orientation-from-image.html?90       image-orientation-ref.html?90
>+fuzzy(1,1) == image-orientation-from-image.html?180      image-orientation-ref.html?180
>+fuzzy(1,1) == image-orientation-from-image.html?270      image-orientation-ref.html?270
>+fuzzy(1,1) == image-orientation-from-image.html?0&flip   image-orientation-ref.html?0&flip
>+fuzzy(1,1) == image-orientation-from-image.html?90&flip  image-orientation-ref.html?90&flip
>+fuzzy(1,1) == image-orientation-from-image.html?180&flip image-orientation-ref.html?180&flip
>+fuzzy(1,1) == image-orientation-from-image.html?270&flip image-orientation-ref.html?270&flip
[etc]

It's a bit unfortunate that all of these tests are fuzzy(1,1). Is that for all platforms, and is it always the same pixel that's fuzzy?

It'd be worth adding a comment above this block of tests, regarding the fuzziness, or better filing a followup bug and noting it here.

Comment 34

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

(In reply to Daniel Holbert [:dholbert] (lowered responsiveness through 8/6) from comment #33)
> It's a bit unfortunate that all of these tests are fuzzy(1,1). Is that for
> all platforms, and is it always the same pixel that's fuzzy?

This is why I dislike doing reftests involving JPEG images. Despite being saved at the maximum quality I possibly could, the regions of solid color are not perfectly uniform. It is true for all platforms; it's not a rendering error.

I'll add a comment.

Comment 35

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

Thanks for the review, Daniel! Great comments! Some responses to a few of them below:

(In reply to Daniel Holbert [:dholbert] (lowered responsiveness through 8/6) from comment #32)
> I also think it'd be worth splitting this patch into two atomic parts:

Yes, that's a good idea! Will do.

> As elegant as this block looks, it's not very debuggable/breakpointable,

Remind me to rant about this at some point when we're together in person. =)

> (Alternately, you could
> just give |angle| a default value up top - but that's probably hackier & a
> bit wasteful (double-setting it in all the normally-traversed cases)).

This branch cannot possibly be taken - the branches of the |if| are exhaustive - but the compiler can't prove it.

However, as a fan myself of establishing correctness statically through the type system, I suspect it might be better to replace the IsXXXDegrees functions with a single function returning an enum, and use a switch here. I originally wrote the code using the IsXXX functions rather than an enum in an attempt to solve a problem that no longer exists. Given that there's no need for this structure anymore, it'd be a quick change, so I'll make it. This way the compiler can prove that the switch cases are exhaustive.

> I'm not clear on why we need to add the |mImage| null-check here,

Good catch! Null-checking |mImage| doesn't make sense there, of course; it should be null-checking |image|. Same goes for the assertion above. This probably clarifies some of the other things that seemed to be wrong in that method. =)

> Hmm, so this is removing a lot of status-checking.

I'm pretty confident that the checks in EnsureIntrinsicSizeAndRatio do not add value. To the extent that those checks were useful, they duplicate checks that are happening inside e.g. UpdateIntrinsicSize and UpdateIntrinsicRatio.

> Same goes for DisplayAltFeedback() and BuildDisplayList(), too - the patch
> removes a lot of status-checking there that could probably be converted into
> an assertion or two, to make sure we're honoring the existing assumptions.

For these two I agree. I'll add some assertions there. I also slightly changed the behavior of DisplayAltFeedback intentionally, but I'm now having second thoughts about that, so I'll restore a check that the image is fully decoded (as opposed to partially decoded).

Comment 36

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

(In reply to Seth Fowler [:seth] from comment #35)
> I also slightly
> changed the behavior of DisplayAltFeedback intentionally, but I'm now having
> second thoughts about that, so I'll restore a check that the image is fully
> decoded (as opposed to partially decoded).

No wait, now I remember why I changed this. The current behavior doesn't make sense because BuildDisplayList will display the image if the first frame is partially decoded, but if it wasn't available when BuildDisplayList was called so that we end up calling DisplayAltFeedback when painting instead, then DisplayAltFeedback discovers that the first frame is now partially decoded, it won't actually paint it unless the first frame is completely decoded. Whether the image gets displayed before the first frame is fully decoded is therefore determined by a race on the timing of the call to BuildDisplayList.

Comment 37

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

(What I should probably do is pull that change out into a separate bug which this one then depends on. I don't want to do it _after_ this bug since it increases the amount of code I have to uselessly write.)

Comment 38

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

I pulled that change out into bug 901146.

Comment 39

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

Attachment: (Part 1) - Add CSS support for the image-orientation property.

Replace the boolean IsXXXAngle functions with a single getter that returns an Angle value.

Comment 40

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

Attachment: (Part 2) - Make nsImageFrame hold a reference to its image container.

This is the portion of the old part 2 related to making nsImageFrame hold a reference to its image container.

Comment 41

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

Attachment: (Part 3) - Add support for image-orientation to nsImageFrame.

This is the portion of the old part 2 that dealt with actually adding image-orientation support to nsImageFrame. Between these two new parts, the issues raised in the review comments should be resolved.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Seth Fowler [:seth] from comment #36)
> No wait, now I remember why I changed this. The current behavior doesn't
> make sense because BuildDisplayList will display the image if the first
> frame is partially decoded, but if it wasn't available when BuildDisplayList
> was called so that we end up calling DisplayAltFeedback when painting
> instead, then DisplayAltFeedback discovers that the first frame is now
> partially decoded, it won't actually paint it unless the first frame is
> completely decoded. Whether the image gets displayed before the first frame
> is fully decoded is therefore determined by a race on the timing of the call
> to BuildDisplayList.

I don't think display list are that durable; in general they're constructed right before they're painted.  We keep them around to do invalidation for the next time.  (At least, as long as there haven't been more changes than I thought with display-list-based invalidation.)

Comment 43

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

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #42)
> I don't think display list are that durable; in general they're constructed
> right before they're painted.

Regardless, it would not have made sense for the logic to be different in the two cases, in my view. However, Timothy pointed out quite rightly that that particular piece of code refers to the loading of the _icon_ which is displayed together with the alt text, and not to the image itself. I've removed that change from the patch; bug 901146 is now about updating the comment there to better reflect what's happening.

Comment 44

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

Attachment: (Part 2) - Make nsImageFrame hold a reference to its image container.

Updated part 2, removing the changes to DisplayAltFeedback. Timothy, could you look this over and let me know if you see any other issues? In general I've predicated some of the changes on the invariant that if mImage is non-null then we have a size, as enforced by OnStartContainer and NotifyNewCurrentRequest.

Comment 45

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

Attachment: (Part 4) - Add image-orientation reftests for raster images.

Updated commit message to reflect part 2 being split in two, and the new reviewer.

Comment 46

[Timothy Nikkel (:tnikkel)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #42)
> I don't think display list are that durable; in general they're constructed
> right before they're painted.

Correct, but even though we don't keep them around that long I think it is possible for image decoding to make progress during painting. For example StartDecoding is called during painting in some cases and it does some decoding, RequestDecode will "accept" decoding progress that has been made off main thread, and we also sync decode images during painting in some cases.

Comment 47

[Timothy Nikkel (:tnikkel)]

Comment on attachment 785320 [details] [diff] [review]
(Part 2) - Make nsImageFrame hold a reference to its image container.

> nsresult
> nsImageFrame::OnStartContainer(imgIRequest *aRequest, imgIContainer *aImage)
> {
>-  if (!aImage) return NS_ERROR_INVALID_ARG;
>+  if (!aImage) {
>+    mImage = nullptr;
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  mImage = aImage;

Doesn't OnStartContainer get called with aImage different from our current request? In which case we need more conditions on assigning aImage to mImage.

Comment 48

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

(In reply to Timothy Nikkel (:tn) from comment #47)
> Doesn't OnStartContainer get called with aImage different from our current
> request? In which case we need more conditions on assigning aImage to mImage.

Yes, this is a good point. It seems that if we get notifications for the current request, this is always the first request the nsImageFrame deals with (not 100% correct because if we change requests before we ever got OnStartContainer then we just treat the new request as current immediately, according to nsImageLoadingContent::PrepareNextRequest), while pending requests are subsequent loads. This design seems intended to ensure that we keep displaying the old image during the process of loading a new one, so we don't revert to a period of just displaying nothing.

In the case where we're getting notifications for a pending request, it seems that OnStopRequest will call NotifyNewCurrentRequest, which already unconditionally updates mImage. I think, based on the inferred purpose of this design, that we do not want to update mImage in OnStartContainer unless we get past the IsPendingLoad check, which ensures that the notification came from the current request.

I'll update a new version of the patch with this change made.

Comment 49

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

Attachment: (Part 2) - Make nsImageFrame hold a reference to its image container.

This version of the patch doesn't update mImage in OnStartContainer until we've established that the notification is for the current request, per comment #48.

Comment 50

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

Attachment: (Part 3) - Add support for image-orientation to nsImageFrame.

This is a rebase of part 3 against the updated part 2.

Comment 51

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

Attachment: (Part 1) - Add CSS support for the image-orientation property.

Rebase against current m-c.

Comment 52

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

Attachment: (Part 4) - Add image-orientation reftests for raster images.

Addressed review comments and added one additional test that ensures that image-orientation doesn't apply to list-style-image, after discussion with fantasai to verify that that's correct.

Comment 53

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

Attachment: (Part 5) - Add image-orientation reftests for SVG images.

The long-awaited final part of this bug! =) This part adds reftests for SVG images.

Comment 54

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

Attachment: (Part 5) - Add image-orientation reftests for SVG images.

Fixed a bug in the reftests for SVG images with no inherent size. I originally thought that the reftests showed a bug in the image-orientation code, which is why I uploaded them anyway, but after investigation it quickly became clear that the reftests themselves were at fault. Other than that bugfix this is the same patch as before.

Comment 55

[Daniel Holbert [:dholbert]]

Comment on attachment 787930 [details] [diff] [review]
(Part 5) - Add image-orientation reftests for SVG images.

>+# Tests for image-orientation with a viewbox and an intrinsic size:
>+== image-orientation-viewbox-and-size.html?0        image-orientation-ref.html?0
>+== image-orientation-viewbox-and-size.html?90       image-orientation-ref.html?90
>+== image-orientation-viewbox-and-size.html?180      image-orientation-ref.html?180
[etc]

So this makes me think: I can totally envision someone making a mistake when patching this test & reference down the line, and accidentally breaking them such that they don't honor their URL parameters. This would make them continue to pass, but no longer test any interesting orientations.

It'd probably worth adding a sanity-check chunk to reftest.list, so that we'll catch that if it happens.  We can just test the reference, to be sure its various reference renderings you're relying on are actually different.  Something like this should do it:

# SANITY-CHECKS
# Make sure the various rotation all look different from each other:
!= image-orientation-ref.html?0   image-orientation-ref.html?90
!= image-orientation-ref.html?0   image-orientation-ref.html?180
!= image-orientation-ref.html?0   image-orientation-ref.html?270
!= image-orientation-ref.html?90  image-orientation-ref.html?180
!= image-orientation-ref.html?90  image-orientation-ref.html?270
!= image-orientation-ref.html?180 image-orientation-ref.html?270
# Make sure "flip" actually changes the rendering:
!= image-orientation-ref.html?0    image-orientation-ref.html?0&flip
!= image-orientation-ref.html?90   image-orientation-ref.html?90&flip
!= image-orientation-ref.html?180  image-orientation-ref.html?180&flip
!= image-orientation-ref.html?270  image-orientation-ref.html?270&flip

Comment 56

[Daniel Holbert [:dholbert]]

Comment on attachment 787930 [details] [diff] [review]
(Part 5) - Add image-orientation reftests for SVG images.

r=me with comment 55

Comment 57

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

Attachment: (Part 4) - Add image-orientation reftests for raster images.

Thanks for the review, Daniel! I think that's a great suggestion. Here's an update of part 4 with sanity checks added.

Comment 58

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

Attachment: (Part 5) - Add image-orientation reftests for SVG images.

And here's part 5 with the sanity checks.

Comment 59

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

Attachment: (Part 1) - Add CSS support for the image-orientation property.

This updated version of part 1:

* Adds a pref to make it possible to disable image-orientation. Right now it
defaults to enabled.

* Adds tests for image-orientation to property_database.js.

Comment 60

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

Try job for the entire patch stack here:

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

Comment 61

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

Attachment: (Part 2) - Make nsImageFrame hold a reference to its image container.

Reuploading due to reports of patch failures against m-c.

Comment 62

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

Attachment: (Part 1) - Add CSS support for the image-orientation property.

Fix errors in property_database.js entry.

Comment 63

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

Attachment: (Part 3) - Add support for image-orientation to nsImageFrame.

Rebased to reflect that Flip::None got renamed to Flip::Unflipped because X.h #define's None. (Sigh.)

Comment 64

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

Hopefully the build issues are dealt with. (They seem to be on my local box, at least.) Some silly test failures are also dealt with. Let's run this through try again.

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

Comment 65

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

I fixed a Windows linker issue in bug 869723 and added some debug output to the nsImageFrame assertion that's causing some oranges in the last try run. Time for another go.

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

Comment 66

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

Attachment: (Part 3) - Add support for image-orientation to nsImageFrame.

This version of part 3 handles some corner cases that were causing oranges.

At this point the patches in this bug look solid on try:

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

Comment 67

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

Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/25da4a357fb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/80cbb3a68078
https://hg.mozilla.org/integration/mozilla-inbound/rev/b05a7a7055de
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd60f3f89bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf995093a329

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25da4a357fb9
https://hg.mozilla.org/mozilla-central/rev/80cbb3a68078
https://hg.mozilla.org/mozilla-central/rev/b05a7a7055de
https://hg.mozilla.org/mozilla-central/rev/dbd60f3f89bd
https://hg.mozilla.org/mozilla-central/rev/cf995093a329

Comment 69

[Jean-Yves Perrier [:teoli]]

Documented:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/26
https://developer.mozilla.org/en-US/docs/Web/CSS/image-orientation

Comment 70

[Asa Dotzler [:asa]]

Release note could be something like "Firefox now supports CSS3's image orientation property."

Comment 71

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Given that I'd like to stick to the "there is no such thing as CSS3" message, how about "CSS3's" -> "the CSS"?

Comment 72

[Jean-Yves Perrier [:teoli]]

And we didn't implement the version as in CSS Image Level 3 but the one in the draft of CSS Image Content Level 4 :-) [ http://dev.w3.org/csswg/css-images/ ] (EXIF support)

Comment 73

[newmanw10]

Is this 26+?  Testing in 26.0 (image-orientation: from-image) and not working.   Pulled nightly build and it works just fine.