Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v4]
On Fri, 7 Jan 2022 00:01:24 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Added test for image format without signature
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java
> line 490:
>
>> 488: if (stream.read(header) <= 0) {
>> 489: return null;
>> 490: }
>
> What was the reason for this change? The former would work even if the stream
> returned less data that the size of the header in a single read, whereas the
> latter would fail.
Well, almost. `ImageTools.readFully` throws `EOFException` if the input stream
is empty. That doesn't make a lot of sense in the context of
`getLoaderBySignature`, since the method should return `null` if the input
stream doesn't contain enough data to detect a signature (and it doesn't seem
useful to me to make a distinction between "not enough" bytes and zero bytes).
I've opted to retain the use of `ImageTools.readFully`, but catch the
`EOFException` and return `null` instead.
-
PR: https://git.openjdk.java.net/jfx/pull/676
Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v4]
On Wed, 24 Nov 2021 03:43:42 GMT, Michael Strauß wrote:
>> `com.sun.javafx.iio.ImageStorage` currently ignores the MIME image subtype
>> specified for images encoded in data URIs. This should be improved as
>> follows:
>>
>> 1. If the specified image subtype is not supported, an exception will be
>> thrown.
>> 2. If the specified image subtype is supported, but the data contained in
>> the URI is of a different (but also supported) image format, the image will
>> be loaded and a warning will be logged. For example, if the MIME type is
>> "image/jpeg", but the image data is PNG, the following warning will be
>> generated:
>>
>>
>> Image format 'PNG' does not match MIME type 'image/jpeg' in URI
>> 'data:image/jpeg;base64,iVBORw0KGgoAAA...AAAElFTkSuQmCC'
>>
>>
>> Also, the javadoc of `javafx.scene.image.Image` incorrectly states:
>>
>> 94* If a URL uses the "data" scheme, the data must be base64-encoded
>> 95* and the MIME type must either be empty or a subtype of the
>> 96* {@code image} type.
>>
>> However, omitting the MIME type of a data URI is specified to imply
>> "text/plain" (RFC 2397, section 2). Since the `com.sun.javafx.util.DataURI`
>> class is implemented according to this specification, trying to load an
>> image without MIME type correctly fails with an `ImageStorageException`:
>> "Unexpected MIME type: text".
>>
>> The solution is to fix the documentation:
>>
>> 94* If a URL uses the "data" scheme, the data must be
>> base64-encoded
>> - 95* and the MIME type must either be empty or a subtype of the
>> - 96* {@code image} type.
>> + 95* and the MIME type must be a subtype of the {@code image} type.
>> + 96* The MIME type must match the image format of the data
>> contained in
>> + 97* the URL. In case of a mismatch between MIME type and image
>> format,
>> + 98* the image will be loaded if the image format can be determined
>> by
>> + 99* JavaFX, and a warning will be logged.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Added test for image format without signature
The API doc change looks good to me. Go ahead and finalize the CSR.
The fix and test look good with one question about what looks like an unrelated
change.
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line
490:
> 488: if (stream.read(header) <= 0) {
> 489: return null;
> 490: }
What was the reason for this change? The former would work even if the stream
returned less data that the size of the header in a single read, whereas the
latter would fail.
-
PR: https://git.openjdk.java.net/jfx/pull/676
Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v4]
> `com.sun.javafx.iio.ImageStorage` currently ignores the MIME image subtype
> specified for images encoded in data URIs. This should be improved as follows:
>
> 1. If the specified image subtype is not supported, an exception will be
> thrown.
> 2. If the specified image subtype is supported, but the data contained in the
> URI is of a different (but also supported) image format, the image will be
> loaded and a warning will be logged. For example, if the MIME type is
> "image/jpeg", but the image data is PNG, the following warning will be
> generated:
>
>
> Image format 'PNG' does not match MIME type 'image/jpeg' in URI
> 'data:image/jpeg;base64,iVBORw0KGgoAAA...AAAElFTkSuQmCC'
>
>
> Also, the javadoc of `javafx.scene.image.Image` incorrectly states:
>
> 94* If a URL uses the "data" scheme, the data must be base64-encoded
> 95* and the MIME type must either be empty or a subtype of the
> 96* {@code image} type.
>
> However, omitting the MIME type of a data URI is specified to imply
> "text/plain" (RFC 2397, section 2). Since the `com.sun.javafx.util.DataURI`
> class is implemented according to this specification, trying to load an image
> without MIME type correctly fails with an `ImageStorageException`:
> "Unexpected MIME type: text".
>
> The solution is to fix the documentation:
>
> 94* If a URL uses the "data" scheme, the data must be base64-encoded
> - 95* and the MIME type must either be empty or a subtype of the
> - 96* {@code image} type.
> + 95* and the MIME type must be a subtype of the {@code image} type.
> + 96* The MIME type must match the image format of the data contained
> in
> + 97* the URL. In case of a mismatch between MIME type and image
> format,
> + 98* the image will be loaded if the image format can be determined
> by
> + 99* JavaFX, and a warning will be logged.
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
Added test for image format without signature
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/676/files
- new: https://git.openjdk.java.net/jfx/pull/676/files/0167532b..4cb5678a
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx=676=03
- incr: https://webrevs.openjdk.java.net/?repo=jfx=676=02-03
Stats: 89 lines in 8 files changed: 51 ins; 3 del; 35 mod
Patch: https://git.openjdk.java.net/jfx/pull/676.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/676/head:pull/676
PR: https://git.openjdk.java.net/jfx/pull/676
