Closed Bug 1160200 Opened 5 years ago Closed 3 years ago

Add image/apng MIME type so it can be used with `picture` type switching.

Categories

(Core :: General, defect)

39 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed
firefox51 --- fixed

People

(Reporter: david, Assigned: marcosc)

References

(Depends on 2 open bugs, )

Details

(Keywords: dev-doc-needed, Whiteboard: good first bug)

Attachments

(1 file, 9 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36

Steps to reproduce:

````
<picture>
	<source type="image/x-apng" srcset="animated.apng" />
	<img src="static.png" />
</picture>
````

(…or `video/png`, `image/apng`, etc.)


Actual results:

`static.png` was displayed


Expected results:

`animated.apng` should have been displayed.
Firefox doesn’t recognize any MIME types for APNG that are distinct from those for regular PNG. Since it doesn’t have a unique MIME type, APNG can’t be used reliably as an alternate format in `picture` file type switching. This becomes extra problematic if an animation is required. Given:

<picture>
	<source type="image/png" srcset="animated.apng" />
	<img src="animated.gif" />
</picture>

…a non-APNG supporting browser will show the APNG as a static image, rather than showing the fallback GIF.
is "type="image/x-apng" a valid type? I thought they were both image/png?

I think the spec should define it's MIME type first. It doesn't seem to do that.
Flags: needinfo?(pavlov)
Nah, `image/x-apng` is not valid, I just using it as an example. Sorry that wasn’t clear. My understanding is that `x-` MIME types are specifically for non-standard things. Really, any MIME type other than `image/png` would be great. And I agree, ideally this would be defined in the spec.
FWIW this would just-work with <picture> if there were an APNG specific MIME type registered.
@johns, where do we have the registrations? Could easily add that.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(pavlov)
Attached patch Add image/apng MIME type (obsolete) — Splinter Review
Attachment #8600216 - Flags: review?(john)
Comment on attachment 8600216 [details] [diff] [review]
Add image/apng MIME type

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

This looks fine with nits below, but since we're adding a new MIME type to imagelib, I think :seth will need to do the review here

::: image/src/imgLoader.cpp
@@ +1354,4 @@
>      mAcceptHeader = accept;
>    } else {
>      mAcceptHeader =
> +        IMAGE_PNG "," IMAGE_APNG "," IMAGE_WILDCARD ";q=0.8," ANY_WILDCARD ";q=0.5";

I'm not sure if we want to include apng here, but I'm not sure what contexts this header is used in.

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +512,4 @@
>    { IMAGE_ICO, "ico,cur", "ICO Image" },
>    { IMAGE_JPEG, "jpeg,jpg,jfif,pjpeg,pjp", "JPEG Image" },
>    { IMAGE_PNG, "png", "PNG Image" },
> +  { IMAGE_APNG, "apng", "APNG Image" },

Is the ".apng" extension something from the spec?
Attachment #8600216 - Flags: review?(seth)
Attachment #8600216 - Flags: review?(john)
Attachment #8600216 - Flags: feedback+
Assignee: nobody → david
Component: Untriaged → General
Product: Firefox → Core
(In reply to John Schoenick [:johns] from comment #7)
>
> ::: image/src/imgLoader.cpp
> @@ +1354,4 @@
> >      mAcceptHeader = accept;
> >    } else {
> >      mAcceptHeader =
> > +        IMAGE_PNG "," IMAGE_APNG "," IMAGE_WILDCARD ";q=0.8," ANY_WILDCARD ";q=0.5";
> 
> I'm not sure if we want to include apng here, but I'm not sure what contexts
> this header is used in.

I wasn’t sure about that bit either. Happy to remove if necessary.


> 
> ::: uriloader/exthandler/nsExternalHelperAppService.cpp
> @@ +512,4 @@
> >    { IMAGE_ICO, "ico,cur", "ICO Image" },
> >    { IMAGE_JPEG, "jpeg,jpg,jfif,pjpeg,pjp", "JPEG Image" },
> >    { IMAGE_PNG, "png", "PNG Image" },
> > +  { IMAGE_APNG, "apng", "APNG Image" },
> 
> Is the ".apng" extension something from the spec?

Nope, the spec [1] doesn’t mention a file extension. The PNG spec [2] that is referenced by APNG states that “On systems where file names customarily include an extension signifying file type, the extension ‘.png’ is recommended for PNG files,” but this isn’t a requirement.

The ‘.apng’ extension is mentioned (without sourcing) on Wikipedia [3] and the File Formats Wiki [4], though. If people use either of those as references, they might create files with a ‘.apng’ extension, so I figured it might be good to register it here.

[1] https://wiki.mozilla.org/APNG_Specification
[2] http://www.w3.org/TR/PNG/
[3] http://en.wikipedia.org/wiki/APNG
[4] http://fileformats.wikia.com/wiki/Animated_Portable_Network_Graphics
Contains relevant discussion: https://bugzilla.mozilla.org/show_bug.cgi?id=257263
(In reply to Dave Newton from comment #9)
> Contains relevant discussion:
> https://bugzilla.mozilla.org/show_bug.cgi?id=257263

Yeah, that bug is excellent reading. Kinda sad that this issue could've been avoided years ago. =\

I strongly support fixing the problem in this bug, but some of the details here need a bit of thought.

In particular, we can't just use "image/apng" without following the appropriate processes - that kind of MIME type needs to be registered, and AFAIK nobody has done that.

As I understand it, we _could_ use "image/x-apng", or perhaps "video/x-png", right away. Those don't require registration. We can then look into registering the non-"x-" variants. I'm not experienced with the MIME type registration process, though, so I may not have the details correct here.

Glenn, is that correct? What do you think we should do here?
Flags: needinfo?(glennrp+bmo)
I have not looked at IANA registration requirements since about 15 years ago.  Unless things have changed, video/x-apng would work ....checking..... The rules are in RFC 6838 (Jan 2013) now, http://tools.ietf.org/html/rfc6838 which strongly discourages the use of "x-" MIME types.  I think MIME type "vnd.mozilla.video.apng" would work; both the vendor name "mozilla" (if not already registered) and the type vnd.mozilla.video.apng (and/or vnd.mozilla.video.png, if you still insist on hijacking the PNG suffix) would have to be registered.
Flags: needinfo?(glennrp+bmo)
Browsing IANA stuff (www.iana.org/assignments/media-types/media-types.xhtml) a little more, I found that "mozilla" already exists as a "vendor" ID, so vnd.mozilla is OK to use.  Also, I had the MIME type facet order slightly wrong; should be video/vnd.mozilla.apng instead of vnd.mozilla.video.apng.
I’d be strongly in favour of registering this as "video/vnd.mozilla.apng" (the registration form is at http://www.iana.org/form/media-types). My gut says that this should be accompanied by a change to the spec, though; Stuart, is that something that would have support from the APNG spec authors? If not, is accepting "video/x-apng" in `picture` the only way to resolve this?
updated patch to use video/vnd.mozilla.apng MIME type
Attachment #8600216 - Attachment is obsolete: true
Attachment #8600216 - Flags: review?(seth)
Attachment #8601215 - Flags: review?(seth)
Stuart, I'd love to hear your thoughts on comment 13.
Flags: needinfo?(pavlov)
Comment on attachment 8601215 [details] [diff] [review]
updated to video/vnd.mozilla.apng

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

Few nits (tho fine if other tests already use xhtml and similar spacing), but lgtm.

::: layout/reftests/apng-mime/expected.html
@@ +1,3 @@
> +<!doctype html>
> +<html>
> +	<title>apng expected</title>

nit: why so much whitespace? you can probably also drop the html tags.

@@ +1,4 @@
> +<!doctype html>
> +<html>
> +	<title>apng expected</title>
> +	<img src="animated.apng" />

nit: " />" (not needed)

::: layout/reftests/apng-mime/test.html
@@ +1,5 @@
> +<!doctype html>
> +<html>
> +	<title>apng test</title>
> +	<picture>
> +		<source type="video/vnd.mozilla.apng" srcset="animated.apng" />

nit: more xhtml syntax.
Attachment #8601215 - Flags: review?(seth) → review+
Attachment #8601215 - Attachment is obsolete: true
Attachment #8612875 - Flags: review+
Comment on attachment 8612875 [details] [diff] [review]
Bug 1160200 - Add image/apng MIME type. Updates re: review.

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

Nit: please reintroduce whitespace in nsMimeTypes.h.
Attachment #8612875 - Attachment is obsolete: true
Attachment #8612888 - Flags: review?(mcaceres)
Attachment #8612888 - Flags: review?(mcaceres) → review+
Registered: 


On May 29, 2015 at 2:10:09 PM, IANA MIME Requests via RT (iana-mime@iana.org) wrote:
> To whom it may concern:
> 
> This is an automatically generated message to notify you that we have
> received your request, and it has been recorded in our ticketing
> system with a reference number of 826172. To check the status
> of your request, please see:
> 
> https://tools.iana.org/ticket-status/app
> 
> If you have any problems accessing this page, please contact
> iana@iana.org.
> 
> There is no need to reply to this message right now. IANA staff will
> review your message shortly.
> 
> If this message is in reply to a previously submitted ticket, it is
> possible that the previous ticket has been marked as closed. As we
> review this ticket, we will also review previous correspondence and
> take appropriate action.
> 
> To expedite processing, and ensure our staff can view the full history
> of this request, please make sure you include the follow exact text in
> the subject line of all future correspondence on this issue:
> 
> [IANA #826172]
> 
> You can also simply reply to this message, as this tag is already in
> the subject line.
> 
> Thank you,
> 
> The Internet Assigned Numbers Authority
> iana-mime@iana.org
> 
> ------------------------------------------------------------------------- 
> 
> Name : Marcos Caceres
> 
> Email : mcaceres@mozilla.com
> 
> MIME media type name : Application
> 
> MIME subtype name : Vendor Tree - vnd.mozilla.apng
> 
> Required parameters : None
> 
> Optional parameters :
> None
> 
> Encoding considerations : 7bit
> 
> 
> Security considerations :
> Same as PNG.
> 
> Interoperability considerations :
> An APNG stream is a normal PNG stream as defined in the PNG Specification, with three additional 
> chunk types describing the animation and providing additional frame data.
> 
> To be recognized as an APNG, an `acTL` chunk must appear in the stream before any `IDAT` 
> chunks. The `acTL` structure is described below.
> 
> Published specification :
> https://wiki.mozilla.org/APNG_Specification
> 
> Applications which use this media :
> Web browsers, image processors
> 
> Fragment identifier considerations :
> none
> 
> Restrictions on usage :
> none
> 
> Provisional registration? (standards tree only) :
> Yes.
> 
> 
> Additional information :
> 
> 1. Deprecated alias names for this type : None
> 2. Magic number(s) : 137 80 78 71 13 10 26 10
> 3. File extension(s) : apng, png
> 4. Macintosh file type code : None
> 5. Object Identifiers: None
> 
> See: https://bugzilla.mozilla.org/show_bug.cgi?id=1160200
> 
> Person to contact for further information :
> 
> 1. Name : Stuart Parmenter
> 2. Email : mcaceres@mozilla.com
> 
> Intended usage : Common
> It is intended to be a replacement for simple animated images that have traditionally 
> used the GIF format, while adding support for 24-bit images and 8-bit transparency. 
> APNG is a simpler alternative to MNG, providing a spec suitable for the most common usage 
> of animated images on the Internet.
> 
> Author/Change controller : Stuart Parmenter 
> Vladimir Vukicevic 
> Andrew Smith 
> 
> 
> 
> 
>
Ooops, wrong email in contact info. Will get that fixed.
Ok, contact is getting fixed by IANA. 

Also, the Encoding considerations is a bug in their form. Also fixed. 

What's next step here? Do we need a super review or can I check this in?
Summary: APNG can't be used with `picture` type switching → Add image/apng MIME type so it can be used with `picture` type switching.
Comment on attachment 8612888 [details] [diff] [review]
Bug 1160200 - Add image/apng MIME type. Whitespace

Talked to Jeff; he will super review.
Flags: needinfo?(pavlov)
Attachment #8612888 - Flags: superreview?(jmuizelaar)
Comment on attachment 8612888 [details] [diff] [review]
Bug 1160200 - Add image/apng MIME type. Whitespace

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

::: image/imgLoader.cpp
@@ +1315,1 @@
>    }

If we use image/vnd.mozilla.apng we don't need to increase the size of our accept header. Using video for apng but gif seems very weird to me.
Attachment #8612888 - Flags: superreview?(jmuizelaar) → superreview-
"so i guess you could say things are getting pretty serious" :)

Dave, let's get this done. See Jeff's comments above.
Flags: needinfo?(david)
Why did we not register "image/apng"? Given that other browsers are interested in implementing this using a vendor proprietary MIME type seems weird.
+1 for a vendor neutral MIME type.

Also, are there plans to expose said MIME type to `Accept` headers? Ideally, we would want developers to be able to choose between a client-side, <picture> based MIME fallback and a server-side based content negotiation.
I attempted to register "video/png" in April 2007 but was shot down by IANA.  Earlier I had attempted to get "image/apng" in the APNG spec but was shot down by Vlad and Stuart (with that change there's a non-zero chance that the APNG chunks would have been approved by the PNG group).  IANA won't approve "image/apng" if it's not in the APNG spec or in the PNG spec, but there's zero chance of either of those happening now.
There wasn't a strong reason to have a separate MIME type back then, since it was just PNG... I have no problem with "image/apng" given <picture> and fallbacks.  We can add "image/apng" to the APNG spec if that would let the IANA officially assign it?
The assignment by IANA wouldn't be automatic.  But if we put it in the spec they might at least be willing to review it.
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=352a6c9cbdb6
Assignee: david → mcaceres
Attachment #8612888 - Attachment is obsolete: true
Flags: needinfo?(david)
Attachment #8666130 - Flags: superreview?(jmuizelaar)
Attachment #8666130 - Flags: superreview?(jmuizelaar) → superreview+
Keywords: checkin-needed
Jeff, you don't think we should change the MIME type to be vendor neutral? I updated the APNG specification to add the vendor neutral type, FWIW.
Flags: needinfo?(jmuizelaar)
(In reply to Anne (:annevk) from comment #34)
> Jeff, you don't think we should change the MIME type to be vendor neutral? I
> updated the APNG specification to add the vendor neutral type, FWIW.

The vendor neutral MIME type has not been accepted by the IANA so I'm not sure how much of a choice we have.
Flags: needinfo?(jmuizelaar)
Jeff, according to Glenn the only thing blocking that was a lack of mention of image/apng in the specification, which has been rectified. Also, I don't think we should let IANA stop us from doing the right thing here, if it comes to that.
You added to the APNG spec, "APNG can be identified using the image/apng MIME type."

Anne, That's jumping the gun.  It should say, similar to what's in the original PNG spec RFC-2083,
"This specification defines a proposed Internet Media Type image/apng." (but actually
according to RFC-6838 it should be proposed to be "video/apng"; see paragraphs 4.2.2
and 4.2.4).  And it should say that it defines Internet Media Type image/vnd.mozilla.apng
as well, and should mention that the file can be identified with file extensions png or apng,
as was stated in the IANA registration of image/vnd.mozilla.apng.

In the past I would have suggested "video/x-apng", similar to what we did with image/x-png;
however, that's deprecated now by RFC-6648.

What's left is "image/vnd.mozilla.apng" that has been registered.
I see IANA says image/vnd.mozilla.apng
But the patch says video/vnd.mozilla.apng
Please check.
(In reply to Max Stepin from comment #40)
> I see IANA says image/vnd.mozilla.apng
> But the patch says video/vnd.mozilla.apng
> Please check.

D'oh. Fixing.
Attached patch Video is now image... (obsolete) — Splinter Review
Attachment #8666130 - Attachment is obsolete: true
Duplicate of this bug: 1265749
(In reply to Glenn Randers-Pehrson from comment #37)
> What's left is "image/vnd.mozilla.apng" that has been registered.

FWIW I strongly disagree with this and the attached patch. We know that other vendors have implemented and shipped APNG and more are planning on doing so. It's a part of the web. It shouldn't matter what registries and standards say as they can be updated. We should ship what we believe is right, and that is that APNG is a standard and not a proprietary format belonging to Mozilla.
(In reply to Anne (:annevk) from comment #47)
> (In reply to Glenn Randers-Pehrson from comment #37)
> > What's left is "image/vnd.mozilla.apng" that has been registered.
> 
> FWIW I strongly disagree with this and the attached patch. We know that
> other vendors have implemented and shipped APNG and more are planning on
> doing so. It's a part of the web. It shouldn't matter what registries and
> standards say as they can be updated. We should ship what we believe is
> right, and that is that APNG is a standard and not a proprietary format
> belonging to Mozilla.

Happy to change it to -webkit/apng ... I mean image/apng.
Attached patch Change to image/apng (obsolete) — Splinter Review
Attachment #8742655 - Attachment is obsolete: true
Better make image/x-apng.
No, x- is no longer to be used. We don't want another application/x-www-form-urlencoded or X-Frame-Options. It doesn't stand the test of time.
Keywords: checkin-needed
(In reply to Anne (:annevk) from comment #51)
> No, x- is no longer to be used. We don't want another
> application/x-www-form-urlencoded or X-Frame-Options. It doesn't stand the
> test of time.

Agree. Checking in as "image/apng"... we can deal with IANA later.
https://hg.mozilla.org/mozilla-central/rev/68442febc36c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
No longer blocks: 1266817
Depends on: 1266817
Backed this out for almost permafailing added test apng-mime/test.html Android 4.3 API15+ opt

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1bf431eb48
Flags: needinfo?(mcaceres)
Seeing if I can reproduce on Android: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=490054d3582a
Flags: needinfo?(mcaceres)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
So, this doesn't work anymore. I don't know what's changed by `<source>` is not being picked up by <img> for "image/apng".

it does "work" (incorrectly) when the test is set discriminate on jpeg:

```
<picture>
<source type="image/jpg" srcset="animated.apng">
<img>
</picture>
```

But when it's switched back to type="image/apng", there is only white displayed. 

The same with:

```
<picture>
<source type="image/jpg" srcset="animated.apng">
<img src="static.png">
</picture>
```

Discrimination on type does seem to be working because: 

```
<picture>
<source type="invalid/garbage" srcset="animated.apng">
<img src="static.png">
</picture>
```

displays "static.png".

@johns, any suggestions?
Flags: needinfo?(john)
Argh... nm... recompiled and it started working again
Flags: needinfo?(john)
Yeah, it's borked on Android. No idea how to proceed. Happy for someone else to take it over from here.
Assignee: mcaceres → nobody
Whiteboard: good first bug
(In reply to Marcos Caceres [:marcosc] from comment #64)
> Yeah, it's borked on Android. No idea how to proceed. Happy for someone else
> to take it over from here.

Looking at this test:
> <source type="image/apng" srcset="animated.apng">

Wouldn't the reftest fail depending on the number of frames animated.apng manages to render before the harness considers the document complete? I'm not sure reftests will work as expected for animations at all.

Converting this to a mochitest that just asserts that img.currentSrc == animated.apng might be all that is necessary
(In reply to John Schoenick [:johns] from comment #65)
> (In reply to Marcos Caceres [:marcosc] from comment #64)
> > Yeah, it's borked on Android. No idea how to proceed. Happy for someone else
> > to take it over from here.
> 
> Looking at this test:
> > <source type="image/apng" srcset="animated.apng">
> 
> Wouldn't the reftest fail depending on the number of frames animated.apng
> manages to render before the harness considers the document complete? I'm
> not sure reftests will work as expected for animations at all.

Yeah, that's probably what's happening. 

> Converting this to a mochitest that just asserts that img.currentSrc ==
> animated.apng might be all that is necessary

Yeah, that should be good enough. I'll convert it.
Assignee: nobody → mcaceres
So, I'm making redoing the apng as a 2 frame animation that only shows green. Right now, it was showing green and then red, which would be subject to random fails. 

I'll also add the mochitest.
Attached patch Adds mochitest (obsolete) — Splinter Review
Added a couple of simple (mochi) tests. 
Made the apng animate over two frames that are the same (green).
Attachment #8743680 - Attachment is obsolete: true
Attachment #8777685 - Flags: review?(john)
Comment on attachment 8777685 [details] [diff] [review]
Adds mochitest

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

r=me on mochitest addition (Assuming this is carrying over jmuizelaar's sr+ since I'm not a DOM peer)
Attachment #8777685 - Flags: review?(john) → review+
Carrying over r+ from Jeff for the c++ bits.
Attachment #8777685 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f29c9642fbca
APNG can't be used with  type switching. r=mcaceres, r=jrmuizel, r=johns
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f29c9642fbca
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Typo in test_picture_apng.html

<srouce>
(In reply to Max Stepin from comment #74)
> Typo in test_picture_apng.html
> 
> <srouce>

Patch: https://bugzilla.mozilla.org/show_bug.cgi?id=1295434
Depends on: 1483680
You need to log in before you can comment on or make changes to this bug.