JPEG-XL can have animations but currently the decoder decodes only the first frame.
Created attachment 445396 [details] Patch
Created attachment 445397 [details] Patch
<rdar://problem/86077143>
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?
Created attachment 446154 [details] Patch
Created attachment 446157 [details] Patch
(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".
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.
Created attachment 446244 [details] Patch
(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())
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].