Bug 83764 – [Qt] Two canvas tests fail with QT_IMAGE_DECODER=0 setup

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Zoltan Horvath]

fast/canvas/canvas-toDataURL-case-insensitive-mimetype.html

--- /home/zoltan/prog/local/WebKit/WebKitBuild/Release/layout-test-results/fast/canvas/canvas-toDataURL-case-insensitive-mimetype-expected.txt
+++ /home/zoltan/prog/local/WebKit/WebKitBuild/Release/layout-test-results/fast/canvas/canvas-toDataURL-case-insensitive-mimetype-actual.txt
@@ -6,9 +6,6 @@
 PASS tryMimeType('image/png') is 'image/png'
 PASS tryMimeType('iMAge/Png') is 'image/png'
 PASS tryMimeType('IMAGE/PNG') is 'image/png'
-PASS tryMimeType('image/jpeg') is 'image/jpeg'
-PASS tryMimeType('imAgE/jPEg') is 'image/jpeg'
-PASS tryMimeType('IMAGE/JPEG') is 'image/jpeg'
 PASS successfullyParsed is true
 
 TEST COMPLETE

fast/canvas/toDataURL-supportedTypes.html

--- /home/zoltan/prog/local/WebKit/WebKitBuild/Release/layout-test-results/fast/canvas/toDataURL-supportedTypes-expected.txt
+++ /home/zoltan/prog/local/WebKit/WebKitBuild/Release/layout-test-results/fast/canvas/toDataURL-supportedTypes-actual.txt
@@ -4,8 +4,8 @@
 MIME types are the SAME.
 
 Given MIMEType: image/jpeg
-Used MIMEType: image/jpeg
-MIME types are the SAME.
+Used MIMEType: image/png
+MIME types DIFFER.
 
 Given MIMEType: image/gif
 Used MIMEType: image/png


Let's check this out!

[Zoltan Horvath]

Created attachment 136871 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 136871 [details]
Patch

r=me

[Csaba Osztrogonác]

Comment on attachment 136871 [details]
Patch

Clearing flags on attachment: 136871

Committed r113960: <http://trac.webkit.org/changeset/113960>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

Looks ok to me.  If I were doing it, I'd consider changing
   http://trac.webkit.org/browser/trunk/Source/WebCore/platform/MIMETypeRegistry.cpp#L290

to read 
  #elif PLATFORM(QT)

is you need to support all the "encoding" formats you did prior to this change.  If the goal is to reduce the number of supported formats, well I'm fine with that also.

And is canvas/philip/tests toDataURL.jpeg.alpha.html passing or skipped on Qt?

[Zoltan Horvath]

(In reply to comment #5)
> Looks ok to me.  If I were doing it, I'd consider changing
>    http://trac.webkit.org/browser/trunk/Source/WebCore/platform/MIMETypeRegistry.cpp#L290
> 
> to read 
>   #elif PLATFORM(QT)
> 
> is you need to support all the "encoding" formats you did prior to this change.  If the goal is to reduce the number of supported formats, well I'm fine with that also.

Thanks! I need to extend my patch in bug #80400 with MIMETypeRegistry changes. Our goal is to change to webcore imagedecoders and let a fallback case to qtimagedecoder for other types.

> And is canvas/philip/tests toDataURL.jpeg.alpha.html passing or skipped on Qt?

It's not skipped and passing.

[noel gordon]

(In reply to comment #6)
> (In reply to comment #5)
> > Looks ok to me.  If I were doing it, I'd consider changing
> >    http://trac.webkit.org/browser/trunk/Source/WebCore/platform/MIMETypeRegistry.cpp#L290
> > 
> > to read 
> >   #elif PLATFORM(QT)
> > 
> > is you need to support all the "encoding" formats you did prior to this change.  If the goal is to reduce the number of supported formats, well I'm fine with that also.
> 
> Thanks! I need to extend my patch in bug #80400 with MIMETypeRegistry changes. Our goal is to change to webcore imagedecoders and let a fallback case to qtimagedecoder for other types.

That's fine for decoders -- here I'm talking about image "encoders".


> > And is canvas/philip/tests toDataURL.jpeg.alpha.html passing or skipped on Qt?
> 
> It's not skipped and passing.

Good, so many ports get this wrong.  You must pass premultipled color components from the 2d canvas to the JPEG encoder to pass this test and so conform to the <canvas> spec -- for output images that do not have alpha, form a Porter-Duff composite src-over black.

For the 3d WebGL <cavnas>.toDataURL() cases, things are a little more complicated.  Sometimes you must pass premultipled color, and sometimes you must not.  Not sure if the Qt implementation has that ability yet, but there is a test for it in the webgl test suite.  

  fast/canvas/webgl/premultiplyalpha-test.html

Refer to bug 68366.

[Zoltan Horvath]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Looks ok to me.  If I were doing it, I'd consider changing
> > >    http://trac.webkit.org/browser/trunk/Source/WebCore/platform/MIMETypeRegistry.cpp#L290
> > > 
> > > to read 
> > >   #elif PLATFORM(QT)
> > > 
> > > is you need to support all the "encoding" formats you did prior to this change.  If the goal is to reduce the number of supported formats, well I'm fine with that also.
> > 
> > Thanks! I need to extend my patch in bug #80400 with MIMETypeRegistry changes. Our goal is to change to webcore imagedecoders and let a fallback case to qtimagedecoder for other types.
> 
> That's fine for decoders -- here I'm talking about image "encoders".

I would modify that when we get rid of QT_IMAGE_DECODER macro, in other case we won't hit elif in L297.

> > > And is canvas/philip/tests toDataURL.jpeg.alpha.html passing or skipped on Qt?
> > 
> > It's not skipped and passing.
> 
> Good, so many ports get this wrong.  You must pass premultipled color components from the 2d canvas to the JPEG encoder to pass this test and so conform to the <canvas> spec -- for output images that do not have alpha, form a Porter-Duff composite src-over black.
> 
> For the 3d WebGL <cavnas>.toDataURL() cases, things are a little more complicated.  Sometimes you must pass premultipled color, and sometimes you must not.  Not sure if the Qt implementation has that ability yet, but there is a test for it in the webgl test suite.  
> 
>   fast/canvas/webgl/premultiplyalpha-test.html
> 
> Refer to bug 68366.

Unfortunately, we skip all webgl tests for Qt-port at http://trac.webkit.org/browser/trunk/LayoutTests/platform/qt/Skipped#L385

It would be good to check these test why they are failing (only few passes).

[noel gordon]

(In reply to comment #8)

> > > Thanks! I need to extend my patch in bug #80400 with MIMETypeRegistry changes. Our goal is to change to webcore imagedecoders and let a fallback case to qtimagedecoder for other types.
> > 
> > That's fine for decoders -- here I'm talking about image "encoders".
> 
> I would modify that when we get rid of QT_IMAGE_DECODER macro, in other case we won't hit elif in L297.

Sounds good Zoltan.  Added myself to bug 80400 to follow along.


> > Refer to bug 68366.

> Unfortunately, we skip all webgl tests for Qt-port ...

I see.