Bug 233545 – Support Animated JPEG-XL images

• Component: Images
• Status: RESOLVED FIXED

[Yoshiaki Jitsukawa]

JPEG-XL can have animations but currently the decoder decodes only the first frame.

[Yoshiaki Jitsukawa]

Created attachment 445396 [details]
Patch

[Yoshiaki Jitsukawa]

Created attachment 445397 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/86077143>

[Said Abou-Hallawa]

Comment on attachment 445397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445397&action=review

> LayoutTests/platform/wincairo/TestExpectations:278
> +fast/images/animated-jpegxl-loop-count.html [ Skip ]

Since the test is skipped globally, where is it un-skipped? I expect it see marked [Pass] here.

I see the comment "# JPEG XL is disabled until WebKitRequirements is released with libjxl" now. But what is the point in adding the test without enabling libjxl? Can't we just have a feature flag or a internal flag to enable it for layout tests only?

> Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:94
> +        if (!i->isInvalid() && !i->isPartial()) // We don't clear the frame we're currently decoding.

This condition and the comment are confusing. Why do you want to keep the isInvalid() frame? And what does "we're currently decoding" mean? Do we run the decoding on a separate thread?

[Yoshiaki Jitsukawa]

Created attachment 446154 [details]
Patch

[Yoshiaki Jitsukawa]

Created attachment 446157 [details]
Patch

[Yoshiaki Jitsukawa]

(In reply to Said Abou-Hallawa from comment #4)
Thank you for the review!

> Since the test is skipped globally, where is it un-skipped? I expect it see
> marked [Pass] here.
> 
> I see the comment "# JPEG XL is disabled until WebKitRequirements is
> released with libjxl" now. But what is the point in adding the test without
> enabling libjxl? Can't we just have a feature flag or a internal flag to
> enable it for layout tests only?

I've marked the wincairo expectations as [Pass]. Also I changed the comment to simply "JPEG-XL tests" because now we have libjxl in the latest WebKitRequirements.

> 
> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:94
> > +        if (!i->isInvalid() && !i->isPartial()) // We don't clear the frame we're currently decoding.
> 
> This condition and the comment are confusing. Why do you want to keep the
> isInvalid() frame? And what does "we're currently decoding" mean? Do we run
> the decoding on a separate thread?

I fixed it to clear invalid frames in case where it has a valid backing store. 

"we're currently decoding" means that the current frame decoding is interrupted due to input starvation. I clarified the relation between the isPartial() and   "currently decoding".

[Don Olmstead]

Comment on attachment 446157 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446157&action=review

r=me with nit

> Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:154
> +        if (query == Query::FrameCount)
> +            return false; // Continue counting frames.
> +        return true; // In this case the decoder has reached the EOF and needs rewind.

Just do

return query != Query::FrameCount

and add any behavior documentation above it.

[Yoshiaki Jitsukawa]

Created attachment 446244 [details]
Patch

[Yoshiaki Jitsukawa]

(In reply to Don Olmstead from comment #8)
> Just do
> 
> return query != Query::FrameCount
> 
> and add any behavior documentation above it.

Thank you！ I addressed the point.
(Also updated the comment in clearFrameBufferCache())

[EWS]

Committed r286628 (244941@main): <https://commits.webkit.org/244941@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446244 [details].