[webkit-reviews] review requested: [Bug 233113] Implement JPEG XL image decoder using libjxl : [Attachment 444594] Patch (with --binary)

[bugzilla-daemon at webkit.org]

Yoshiaki Jitsukawa <yoshiaki.jitsukawa at sony.com> has asked  for review:
Bug 233113: Implement JPEG XL image decoder using libjxl
https://bugs.webkit.org/show_bug.cgi?id=233113

Attachment 444594: Patch (with --binary)

https://bugs.webkit.org/attachment.cgi?id=444594&action=review




--- Comment #5 from Yoshiaki Jitsukawa <yoshiaki.jitsukawa at sony.com> ---
Created attachment 444594

  --> https://bugs.webkit.org/attachment.cgi?id=444594&action=review

Patch (with --binary)

 - Tests added.

 - Check JPEG XL signature by using JxlSignatureCheck().

> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:46
> > +ScalableImageDecoderFrame* JPEGXLImageDecoder::frameBufferAtIndex(size_t
index)
> > +{
> > +	 if (index)
> > +	     return nullptr;
> 
> Hm, this doesn't seem good? Are you missing a FIXME here? It's only
> implemented for index=0?

 - A TODO comment Added.

> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:125
> > +	     if (status == JXL_DEC_ERROR) {
> > +		 return status;
> > +	     }
> > +	     if (status == JXL_DEC_SUCCESS) {
> > +		 return status;
> > +	     }
> > +	     if (status == JXL_DEC_NEED_MORE_INPUT) {
> > +		 return status;
> > +	     }
> 
> I would probably write it all in one conditional. Regardless of what you
> prefer, be sure to placate the style checker EWS. It's not going to like
> braces around single-line conditionals.

 - Merged into one conditional.

> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:180
> > +	     // FIXME:
> 
> What needs fixed?

 - Set pixel with setPixel() function.