Bug 100698 – Add a test for reading exif orientation off image documents

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

Add a test for reading exif orientation off image documents

[Nico Weber]

Created attachment 171313 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 171313 [details]
Patch

Could we make this say PASS/FAIL instead?  You, the test writer, have that information better than someone bringing up a new port and staring at this for the first time. :)

[Nico Weber]

Created attachment 171334 [details]
Patch

[Nico Weber]

(In reply to comment #2)
> (From update of attachment 171313 [details])
> Could we make this say PASS/FAIL instead?  You, the test writer, have that information better than someone bringing up a new port and staring at this for the first time. :)

Sure thing, done. (I copied the behavior of exif-orientation.html, but I agree that it's better now.)

[Nico Weber]

Created attachment 171335 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 171334 [details]
Patch

I would have done something like expectHorizontal(1); expectVertical(2); for each of the cases.  But this is fine too.

[WebKit Review Bot]

Comment on attachment 171335 [details]
Patch

Rejecting attachment 171335 [details] from commit-queue.

New failing tests:
platform/chromium/virtual/deferred/fast/images/exif-orientation-image-document.html
Full output: http://queues.webkit.org/results/14631303

[WebKit Review Bot]

Comment on attachment 171334 [details]
Patch

Attachment 171334 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14623550

New failing tests:
platform/chromium/virtual/deferred/fast/images/exif-orientation-image-document.html

[Nico Weber]

Created attachment 171364 [details]
Patch for landing

[Nico Weber]

hclam: The test I'm adding in this CL fails with in the deferred image decoding suite. I'm adding it to TestExpectations to the block of other failing tests. It'll need to be fixed before deferred decoding can become turned on by default.

[Hin-Chung Lam]

(In reply to comment #10)
> hclam: The test I'm adding in this CL fails with in the deferred image decoding suite. I'm adding it to TestExpectations to the block of other failing tests. It'll need to be fixed before deferred decoding can become turned on by default.

okay sounds good to me.

[WebKit Review Bot]

Comment on attachment 171364 [details]
Patch for landing

Rejecting attachment 171364 [details] from commit-queue.

New failing tests:
platform/chromium/virtual/deferred/fast/images/exif-orientation-image-document.html
Full output: http://queues.webkit.org/results/14642307

[Nico Weber]

Created attachment 171368 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 171368 [details]
Patch for landing

Clearing flags on attachment: 171368

Committed r132877: <http://trac.webkit.org/changeset/132877>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

Chromium Expectations added in r132989 <http://trac.webkit.org/changeset/132989>

[Dominik Röttsches (drott)]

Nico, are you sure this should work without overriding the preference? My suspicion is, it passes on Chromium since the preference is default on, isn't it?

I would suggest to add
 testRunner.overridePreference('WebKitShouldRespectImageOrientation', 1);

[Nico Weber]

Hi Dominik,

(In reply to comment #17)
> Nico, are you sure this should work without overriding the preference?

Yes, that's correct: The preference controls if exif information is used for <img> elements in html documents. For image documents, exif information is always used on ports that support it.

> My suspicion is, it passes on Chromium since the preference is default on, isn't it?

No, the pref is off by default in chromium, and we don't plan to turn it on in the close future.

> I would suggest to add
>  testRunner.overridePreference('WebKitShouldRespectImageOrientation', 1);

That won't have an effect on the test.


For the exif stuff to work, your port needs to have explicit code to support drawing of rotated images. For that reason, RenderObject::shouldRespectImageOrientation() has an #ifdef for the imagedocument code that's currently only on for mac and chromium. If your port's drawing code doesn't support honoring image orientation, then without that ifdef images would be laid out according to their exif information, but then only stretched (instead of rotated) at paint time – which is worse than them being 90 degree rotated. (To see what I mean, remove the ifdef in RenderObject::shouldRespectImageOrientation() and look at the test this bug added in your port). If you want to get this test passing on your port, you need to implement drawing support first and then enable the ifdef for your port.

[Eric Seidel (no email)]

Comment on attachment 171368 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=171368&action=review

Isn't this a text test?

> LayoutTests/platform/chromium/TestExpectations:3829
> +Bug(thakis) fast/images/exif-orientation-image-document.html [ ImageOnlyFailure ] 

image?

[Nico Weber]

(In reply to comment #19)
> (From update of attachment 171368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171368&action=review
> 
> Isn't this a text test?

No. The test says `testRunner.dumpAsText(true);`, and the `true` means "generate pixel dumps" as far as I know.

If your question is "shouldn't it be sufficient to make this a text-only test?": No, because the image bounds alone don't tell you if an image is upside down.

> 
> > LayoutTests/platform/chromium/TestExpectations:3829
> > +Bug(thakis) fast/images/exif-orientation-image-document.html [ ImageOnlyFailure ] 
> 
> image?

[Dominik Röttsches (drott)]

(In reply to comment #18)

Thanks Nico for the detailed explanations.

I did implement the rotation/flipping code in bug 101207 for Cairo, however, i didn't find that RenderObject #ifdef earlier. 

> > I would suggest to add
> >  testRunner.overridePreference('WebKitShouldRespectImageOrientation', 1);
> 
> That won't have an effect on the test.

Actually, it does. If I enable the setting inside the test, it passes on Cairo/EFL since the second line of the if-condition in RenderObject::shouldRespectImageOrientation checks for the setting.

> For the exif stuff to work, your port needs to have explicit code to support drawing of rotated images. For that reason, RenderObject::shouldRespectImageOrientation() has an #ifdef for the imagedocument code that's currently only on for mac and chromium.  [...] If you want to get this test passing on your port, you need to implement drawing support first and then enable the ifdef for your port.

Yep, I'll add USE(CAIRO) to the ifdef in RenderObject that you mentioned. Thanks again.