723931 - regression: toDataURL unable to produce PNG without alpha channel

Status: NEW

(Core :: Graphics: Canvas2D, defect)

Description

[Kathleen :Brade]

Prior to Firefox 7, add-ons could call toDataURL("image/png", "transparency=none") to generate a PNG without an alpha channel.  This technique is used by image capture add-ons such as Page Saver (to produce smaller images).

This regression was introduced with the checkin for bug 564388.

Although the spec doesn't provide any options for PNG images, this functionality is useful. 

I would like guidance on how to proceed.  Would a patch that re-instated support for transparency=none be accepted?  Are there any security considerations that would require that web pages not be allowed to use this option?

Comment 1

[Boris Zbarsky [:bzbarsky]]

One problem is that the spec explicitly requires that arguments after the first one be ignored for image/png and reserves them for later extension points.

It may be worth it to propose a spec change to allow a dictionary argument as the second argument for image/png and then support { moz-transparency: "none" } in there.  Though I guess we could also just accept a second string argument in chrome only; then extensions may need to change if the spec starts using that argument for something....

A last option is to propose a standard way to do this, of course.  Seems like people might want it in general.

Comment 2

[Markus Stange [:mstange]]

Another option would be to check if the moz-opaque attribute is set on the canvas.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Or if all the alpha values are 1.0, we can omit the alpha channel automatically.

Comment 4

[Mark Smith [:mcs]]

(In reply to Boris Zbarsky (:bz) from comment #1)
> A last option is to propose a standard way to do this, of course.  Seems
> like people might want it in general.

Agreed, although I don't know if such a proposal would be accepted and who should make the proposal.  I like roc's idea from comment #3 because it would mean that Firefox would do the right thing automatically in most cases.  Markus' idea (comment #2) is the most appealing because it seems simple to implement and it would allow control over image format to be placed in developer/users' hands.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> although I don't know if such a proposal would be accepted and who should make the
> proposal.

For the former, "who knows".  For the latter, anyone who wants to send mail to [e-mail].

That said, I really like roc's proposal, though I agree that Markus' would be faster to implement.  I'm a little surprised we expose moz_opaque to web content....

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Or if all the alpha values are 1.0, we can omit the alpha channel
> automatically.

I think this would be really easy to implement.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

I just had to prove it :-).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure what the best way is to write an automated test for this. Joe, please advise.

Comment 9

[Joe Drew (not getting mail)]

I'll review this tomorrow, but one way to test it would be to use imgITools; you could encode an image without an alpha channel, then compare the byte stream to what comes out of a load of the data URI.

image/test/unit/test_imgtools.js

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 594876 [details] [diff] [review]
fix

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

::: image/encoders/png/nsPNGEncoder.cpp
@@ +49,5 @@
>  using namespace mozilla;
>  
>  NS_IMPL_THREADSAFE_ISUPPORTS3(nsPNGEncoder, imgIEncoder, nsIInputStream, nsIAsyncInputStream)
>  
> +nsPNGEncoder::nsPNGEncoder() : mPNG(nsnull), mPNGinfo(nsnull), mColorType(0),

Color type of 0 actually corresponds to PNG_COLOR_TYPE_GRAY; maybe initialize to -1 to make sure we always override it. Alternately set it to PNG_COLOR_TYPE_RGB_ALPHA.

@@ +133,5 @@
> +#endif
> +  const PRUint8* alphas = aData + alphaValueOffset;
> +  for (PRUint32 y = 0; y < aHeight; ++y) {
> +    for (PRUint32 x = 0; x < aWidth; ++x) {
> +      if (alphas[x*4] < 0xFF)

!= 0xFF instead please

@@ +216,5 @@
>    if ((aInputFormat == INPUT_FORMAT_HOSTARGB ||
> +       aInputFormat == INPUT_FORMAT_RGBA) &&
> +      useTransparency &&
> +      (!aData ||
> +       !IsImageAlphaChannelAllOpaque(aData, aWidth, aHeight, aStride, aInputFormat)))

This is really, really complex.

It might help if IsImageAlphaChannelAllOpaque null-tested aData (and returned false then, I guess).

Can you add a comment describing what you're doing here? And add {} braces to the bodies too please.

@@ +296,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
>    // parse and check any provided output options
> +  nsresult rv = ParseOptions(aFrameOptions, nsnull, nsnull,

Why not let users override our choice?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Joe Drew (:JOEDREW!) from comment #10)
> @@ +216,5 @@
> >    if ((aInputFormat == INPUT_FORMAT_HOSTARGB ||
> > +       aInputFormat == INPUT_FORMAT_RGBA) &&
> > +      useTransparency &&
> > +      (!aData ||
> > +       !IsImageAlphaChannelAllOpaque(aData, aWidth, aHeight, aStride, aInputFormat)))
> 
> This is really, really complex.

You mean the boolean expression, or the underlying logic?

> Can you add a comment describing what you're doing here? And add {} braces
> to the bodies too please.

Sure.

> @@ +296,5 @@
> >      return NS_ERROR_FAILURE;
> >    }
> >  
> >    // parse and check any provided output options
> > +  nsresult rv = ParseOptions(aFrameOptions, nsnull, nsnull,
> 
> Why not let users override our choice?

Yes, I suppose we probably should.

Comment 12

[Mark Smith [:mcs]]

Robert, any chance you will have time to address the review comments and produce a revised patch?  It would be really nice to have this fix.

Comment 13

[sss.sonik]

Attachment: Proposed fix after bug-982480

Now that bug 982480 has been fixed, we can request a opaque canvas with

> canvas.getContext("2d", { alpha:false });

Attached is a proof-of-concept fix.

Latter part of the patch is necessary,
but this should be splitted into another bug,
because this part addresses the conflicting situation such that

- canvas doesn't have 'moz-opaque' attribute
- and getConetxt() has been called with {alpha: false}