-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Give toDataURL() and toBlob() default parameter values #3477
Give toDataURL() and toBlob() default parameter values #3477
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also require web-platform-tests changes to update the IDL.
source
Outdated
@@ -65451,8 +65450,7 @@ interface <dfn>OffscreenCanvasRenderingContext2D</dfn> { | |||
data-x="js-Type">Type</span>(<var>quality</var>) is Number, and <var>quality</var> is in the range | |||
0.0 to 1.0 inclusive, the user agent must treat <var>quality</var> as the desired quality level. | |||
If <span data-x="js-Type">Type</span>(<var>quality</var>) is not Number, or if <var>quality</var> | |||
is outside that range, the user agent must use its default quality value, as if the | |||
<var>quality</var> argument had not been given.</p> | |||
is outside that range, the user agent must use its default quality value.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should just use 1.0 here as well. @junov?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reservations about this change. This is not backwards compatible and will cause regressions. Changing the default to 1.0 will cause all websites that currently use the default to suddenly create much larger files and to take more time than before. Currently the default is a sensible compromise between quality, compression ratio, and encoding performance. The reason the default value is not standardized is that different browsers use different encoder libraries that each have their own interpretation for this parameter. To overcome this problem, each browser gets to decide what its implementation-specific default should be. This provides web devs with a interoperable way of getting a reasonable quality/compression compromise.
In my opinion we should not specify a default parameter value unless we also standardize the interpretation of the parameter value. If we do go down that road, then we should set the default to something that is backwards compatible (not 1.0).
A way forward for the spec would be to select a reference open source implementation for each format that supports the quality parameter (e.g. libjpeg for image/jpeg), and to state that the interpretation of the quality parameter is the one defined by that reference implementation. Browsers that use a different encoder library may need to apply a conversion to the quality parameter in order to match the interpretation from the reference implementation. Then we could standardize the default parameter value in a backwards compatible way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case why did you specify a default for OffscreenCanvas
? Is that a bug that needs to be fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OffscreenCanvas is a new API, so there is no backward compatibility issue.
PNG, which is the default format, can support a variable quality setting, but historically PNG has been used for lossless compression and developers have an expectation that PNG encoding should be lossless by default, and many browser implementation ignore the quality parameter when encoding PNGs. That is why I set the default to 1.0.
I realize now that there still problem... there's the issue of providing an interoperable quality/compression compromise for JPEG. I guess this needs more thought...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like we should say that for "image/png" we set quality to 1.0 irrespective of its actual value and for other formats the default is user agent defined (and we don't fiddle with the value unless it's out of range or not a number). And we remove the 1.0 default from IDL for OffscreenCanvas and not add it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see:
- Remove
= 1.0
from IDL (including from theImageEncodeOptions
dictionary). - Turn quality into an optional argument again. Only type is always given now. You will also need to change this in
convertToBlob()
(step 6.1). - Change "If type is an image format that supports variable quality" to say something like "If type is an image format that supports variable quality, type is not "image/png", ..." so that even though image/png supports variable quality, quality is not taken into account for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also be nice to have a non-normative note explaining that different implementations may have slightly different interpretations of "quality" and that when quality is not specified an implementation-specific default is used that represents a reasonable compromise between compression ratio, image quality and encoding time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an implementor, agree with @junov. A non-normative note would allow implementors some latitude e.g. to achieve said compromise in the case of the default. Was that added?
In the case on image/png, does opt_quality apply? If so, what does it mean given the image is losslessly encoded? It would be nice if opt_quality could be used to control the compression. 0 - minimal compression and so a fast encode. 1 - maximum compression and a slow encode. default - the implementation-specific compromise b/w compression & encoding time. Adios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://bugs.chromium.org/p/chromium/issues/detail?id=179289 Request to control compression quality for .toDataURL "image/png".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noell that should be a separate issue as that would be a normative change to the standard (image/png has to ignore quality at the moment).
I made some changes, but I'm not sure if they're correct. Would appreciate another review. |
Updated. Does the non-normative section need its own heading? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one more change required elsewhere. The convertToBlob()
definition has this step:
Let file be a serialization of bitmap as a file, with options's type and quality.
You'll need to add " if present" there. We use "if present" rather than "if given" since it's a dictionary member and not an argument.
Updated. |
Thanks! @junov want to do a final pass? |
This will need rebasing after #3926 lands. However I think we editors should do it for you, and merge, because it is silly that we've let this languish for so long. As for tests, I think we won't need any now that the IDL auto-sychronizes from the spec. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@domenic how about we merge this as the other PR isn't moving forward?
Thanks @gloverdonovan and apologies for the long delay on this! |
This pull request fixes #3430. I also removed a comma since I don't think it's needed.
/canvas.html ( diff )