David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Description • 12 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 1 • 12 years ago

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.

	fantasai
	Comment 2 • 12 years ago

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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 3 • 12 years ago

(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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 4 • 12 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 5 • 12 years ago

(from an imagelib perspective, that is)

	Joe Drew (not getting mail)
	Comment 6 • 11 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 7 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 8 • 11 years ago

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

	Fred Wenzel [:wenzel]
	Comment 9 • 11 years ago

(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?

	Cameron McCormack (:heycam)
	Comment 10 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 11 • 11 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 12 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 13 • 11 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 14 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 15 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 16 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 17 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 18 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 19 • 11 years ago

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.)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 20 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 21 • 11 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 22 • 11 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 23 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 24 • 11 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 25 • 11 years ago

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.)

	Daniel Holbert [:dholbert]
	Comment 26 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 27 • 11 years ago

Have fun at the wedding, Daniel!

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 28 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 29 • 11 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 30 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 31 • 11 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 32 • 11 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 33 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 34 • 11 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 35 • 11 years ago

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).

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 36 • 11 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 37 • 11 years ago

(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.)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 38 • 11 years ago

I pulled that change out into bug 901146.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 39 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 40 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 41 • 11 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 42 • 11 years ago

(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.)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 43 • 11 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 44 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 45 • 11 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 46 • 11 years ago

(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.

	Timothy Nikkel (:tnikkel)
	Comment 47 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 48 • 11 years ago

(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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 49 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 50 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 51 • 11 years ago

Rebase against current m-c.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 52 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 53 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 54 • 11 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 55 • 11 years ago

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

	Daniel Holbert [:dholbert]
	Comment 56 • 11 years ago

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

r=me with comment 55

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 57 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 58 • 11 years ago

And here's part 5 with the sanity checks.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 59 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 60 • 11 years ago

Try job for the entire patch stack here:

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 61 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 62 • 11 years ago

Fix errors in property_database.js entry.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 63 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 64 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 65 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 66 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 67 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 68 • 11 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 69 • 11 years ago

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

	Asa Dotzler [:asa]
	Comment 70 • 11 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 71 • 11 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 72 • 11 years ago

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)

	newmanw10
	Comment 73 • 11 years ago

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