w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.44k stars 657 forks source link

[css-images-3] Allow impls to not respect exif data if it's after the image data #4929

Closed tabatkins closed 1 year ago

tabatkins commented 4 years ago

In https://github.com/w3c/csswg-drafts/issues/3799#issuecomment-610443730, @eeeps brings up the fact that none of the browsers currently supporting image-orientation: from-image; respect the orientation if it comes after the image data. (firefox bug, webkit bug, chrome bug)

This is fairly reasonable, as it would be incompatible with streaming display of the image; it would progressively render in one orientation, and then flip around once it's finished. This is not only somewhat jarring, it seems to be incompatible with current browser image loading architectures, without some refactoring of uncertain complexity.

I suggest that we explicitly allow impls to not respect orientation if it comes after the image data; possibly we should restrict them from doing so, tho I'm not sure if this unnecessarily constrains their future usage of image libraries.

chrishtr commented 4 years ago

FYI Chromium supports this now also.

tabatkins commented 4 years ago

Do you just mean "supports image-orientation: from-image;"? What version? Just jpeg, or png also?

In the test files linked from the Firefox bug, I don't see either image rotated in Chrome.

chrishtr commented 4 years ago

Yes, it should be supported... let me check on this.

chrishtr commented 4 years ago

@schenney-chromium

schenney-chromium commented 4 years ago

Loading the "test files linked form the Firefox bug" correctly renders in Chrome Canary. i.e. the dog is right-way-up in both images.

tabatkins commented 4 years ago

...are you sure that's what it's supposed to do? I'm pretty sure the dog image as originally right side up, and if exif is supported, it should be rotated 90deg.

However, that testfile is super unclear, and I've pinged @eeeps to put a "no EXIF" version up so we can tell if things are broken or working correctly.

schenney-chromium commented 4 years ago

Tab, you're right. In Chrome-80, without image-orientation support, the dog is right way up in both images, so that mean Chrome 81+ is not displaying it with expected results.

schenney-chromium commented 4 years ago

If the bug is common everywhere, maybe it's in libjpeg-turbo.

eeeps commented 4 years ago

I'm pretty sure the dog image as originally right side up, and if exif is supported, it should be rotated 90deg.

Correct. Added an EXIF-less PNG to both pages for clarity.

As for the JPEG -- in Canary (83) it's correctly rotated 90°CW; in Stable (80) it is not. Given that image-orientation support was added in 81, this meets my expectations; I don't think there's anything weird going on with the JPEG in any browser.

schenney-chromium commented 4 years ago

Ahh, I thought this was jpeg. If we're talking PNG that's known, https://bugs.chromium.org/p/chromium/issues/detail?id=1067846, but I guess you're the one who reported it. I'm sorry for causing so much confusion - brain mangled after a day of small kids.

tabatkins commented 4 years ago

I've hidden the diversion due to confusion. ^_^

So bringing us back:

Chrome, Safari, and Firefox all now respect image-orientation: from-image on JPGs (in their latest versions, make sure you're updated), as shown in https://ericportis.com/etc/PNG-EXIF-orientation/.

Chrome and Firefox don't currently support it on PNGs at all (but plan to fix that). Safari does support EXIF on PNGs, but only if it comes before the image data (the PNG spec technically allows it to show up before or after). (The test linked above has the EXIF after the image data.)

Orientation data showing up after the image data is hostile to streaming display of the image: it'll progressively render one way, then flip at the very end, and that's assuming your rendering pipeline even allows changing orientation after-the-fact (apparently Safari's doesn't currently).

So! Given that, should the spec allow (or mandate) ignoring image-derived orientation if it comes after the image data?

svgeesus commented 4 years ago

It would be reasonable for the relatively recent (2017) specification for EXIF in PNG to add a recommendation to put EXIF data before the IDAT (image data) chunk, in section 3.7.3 eXIf Recommendations for Encoders.

annevk commented 4 years ago

I think it should be mandated (and tested), otherwise you get a race to the bottom.

css-meeting-bot commented 3 years ago

The CSS Working Group just discussed Allow impls to not respect exif data if it's after the image data, and agreed to the following:

The full IRC log of that discussion <gregwhitworth> Topic: Allow impls to not respect exif data if it's after the image data
<astearns> github: https://github.com/w3c/csswg-drafts/issues/4929
<gregwhitworth> TabAtkins: so the image orientation from image value allows you to use the exif data and rotate accordingly
<gregwhitworth> TabAtkins: some people noted that the location is flexible. If the exif data can live at the end and you can not initially do the orientation if you're progressively rendering
<gregwhitworth> TabAtkins: recommending to not support exif data if the exif data comes after the image data
<gregwhitworth> leaverou: any numbers on how common this is?
<gregwhitworth> TabAtkins: no
<gregwhitworth> TabAtkins: someone mentioned it was less likely, but no strict numbers
<smfr> q+
<gregwhitworth> chris: cameras have that first and the only cases where this will get messed with is the tools that will shove it at the end
<gregwhitworth> TabAtkins: once the image data starts the browser will ignore any other exif data
<smfr> i will type
<smfr> i find it really weird that css would say anything about how images are encoded
<gregwhitworth> astearns: anyone else with a comment on this while we wait for smfr
<smfr> i think we treat this as a “bad image” and just let the bad stuff happen
<gregwhitworth> TabAtkins: while weird, all browsers are doing this
<smfr> also might be hard for implementors: not sure if we can ask our image framework HOW the metadata is ordered
<gregwhitworth> astearns: we're doing bad things now because we haven't found exif prior to rendering
<smfr> what about a small image which is loaded in 1 go and the EXIF data is still after the image data
<gregwhitworth> florian: what about cache scenarios?
<gregwhitworth> iank_: I wouldn't bet on that, it's often sometimes slower
<smfr> would be OK with a MAY
<gregwhitworth> astearns: what if we have UAs may choose to ignore exif data if it isn't before the image data
<smfr> don’t want it to be a MUST because of reasons above
<gregwhitworth> chris: advise authors to avoid putting it at the end since there isn't interop
<gregwhitworth> TabAtkins: then we could go with a should then
<gregwhitworth> hober: smfr says he's ok with may
<gregwhitworth> florian: not having interop is bad too
<TabAtkins> smfr, you okay with SHOULD?
<TabAtkins> (probably with an explicit callout to violations being due to image-decoders not reflecting the ordering)
<smfr> ok with SHOULD
<gregwhitworth> astearns: Should we be "UAs should ignore exif data that comes after image data"
<gregwhitworth> Proposed Resolution: UAs should ignore exif data that comes after image data
<smfr> q-
<gregwhitworth> astearns: I thought loading the image included looking for exif data
<gregwhitworth> TabAtkins: yeah, it's not a precise term
<futhark> [leaving]
<gregwhitworth> RESOLVED: UAs should ignore exif data that comes after image data
<astearns> ack fantasai
<Zakim> fantasai, you wanted to ask if we should also say authors SHOULD NOT use such images?
<gregwhitworth> TabAtkins: I think we should do an authors MUST NOT
<gregwhitworth> fantasai: we should give a clear reason as to why to do that
<fantasai> s/reason/guidance/
<fantasai> s/as to why to/to not/
<gregwhitworth> astearns: if we put in author guidance, we should make sure to put in example of why they shouldn't
<gregwhitworth> Proposed Resolution: Authors MUST NOT put exif data at the end of the image
<gregwhitworth> Resolved: Authors MUST NOT put exif data after the image data
<TabAtkins> oh time to rejoin huh
<TabAtkins> we're having fun in our breakout, new meeting, csswg is passe
<iank_> TabAtkins: do you trigger the rejoin? or do we have to opt-in?
<TabAtkins> there's a "return to main call" button on your video
<iank_> ah saw the big button thingy
<fremy> ScribeNick: fremy

See official minutes

eeeps commented 3 years ago

It might be helpful to differentiate between render-impacting and non-render-impacting metadata, and not be specific about the EXIF metadata format.

Encoders often want to put non-render-impacting metadata after the image data, so that image data is received (and in the case of progressive formats, can be painted) ASAP. See for instance, the suggested order of "extended" WebP metadata: https://developers.google.com/speed/webp/docs/riff_container#extended_file_format

Some formats may go further and mandate, rather than suggest, that EXIF come after. I think JPEG-XL does this? (though, IIRC, it has special-purpose format-specific metadata fields for render-impacting things like orientation and resolution that must come before image data). cc: @jonsneyers

tabatkins commented 3 years ago

That's a good clarification, and one I'm happy to adopt.

annevk commented 3 years ago

I'm not happy with @smfr's example and using that as justification for "should" rather than "must". Perhaps it is out of place for CSS to say something about images, but I think for the web we should have a consistent image model across implementations and it definitely shouldn't depend on the size of the image what ends up happening. That will lead to confusion for web developers.

If the concern is really about CSS defining it, perhaps image fetching and decoding should be in its own document?

tabatkins commented 3 years ago

Can you clarify what makes you unhappy? His justification is that the image decoder in use may simply not have the ability to report that data; as such, making it a MUST requirement would effectively require a switch or rewrite of the image decoder, which seems like a pretty big request to make.

His second reason, about small images, I agree is bad; the size of the image shouldn't have an effect here.

The owner of the requirement won't make any difference to the issues raised here.

annevk commented 3 years ago

Having standardized image decoders across the web platform does not seem like a big request? Why would we accept non-interoperability there?

tabatkins commented 3 years ago

Because the browser vendors in the room expressed their discomfort at that sort of requirement.

jonsneyers commented 3 years ago

In the JPEG XL file format, you can put the Exif data where you want, but for web use cases, we define an abbreviated 'file format' that is just a 'naked' JPEG XL codestream without any metadata.

All render-impacting metadata is defined in the JXL codestream, and if the optional Exif (or XMP) metadata says something that conflicts with what the codestream says, then a decoder has to ignore the Exif/XMP. The codestream is the single source of truth. This includes image dimensions, bit depth, number of channels, image orientation, intrinsic dimensions, and color space. In the codestream these things are part of the header, and are always put before the actual image data in the bitstream. Otherwise you cannot do meaningful progressive decoding.

tabatkins commented 3 years ago

Fixed in a42612866

@smfr Could you review?

zcorpan commented 3 years ago

@tabatkins should this issue be closed?

It looks like https://github.com/web-platform-tests/wpt/pull/26915 tests this. Correct?

svgeesus commented 1 year ago

It looks like the spec edits are done and WPT in place. @fantasai Close?

tabatkins commented 1 year ago

I'd just asked for a review before closing, but after a year and a half of waiting (and WPT enforcing it) I think it's safe to close as completed. ^_^