Bug 83764 - [Qt] Two canvas tests fail with QT_IMAGE_DECODER=0 setup
Summary: [Qt] Two canvas tests fail with QT_IMAGE_DECODER=0 setup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords: Qt
Depends on: 32410
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-12 02:57 PDT by Zoltan Horvath
Modified: 2012-04-14 04:43 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.81 KB, patch)
2012-04-12 03:40 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Horvath 2012-04-12 02:57:44 PDT
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!
Comment 1 Zoltan Horvath 2012-04-12 03:40:16 PDT
Created attachment 136871 [details]
Patch
Comment 2 Csaba Osztrogonác 2012-04-12 03:45:54 PDT
Comment on attachment 136871 [details]
Patch

r=me
Comment 3 Csaba Osztrogonác 2012-04-12 03:52:26 PDT
Comment on attachment 136871 [details]
Patch

Clearing flags on attachment: 136871

Committed r113960: <http://trac.webkit.org/changeset/113960>
Comment 4 Csaba Osztrogonác 2012-04-12 03:52:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 noel gordon 2012-04-12 04:37:52 PDT
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?
Comment 6 Zoltan Horvath 2012-04-12 04:54:07 PDT
(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.
Comment 7 noel gordon 2012-04-12 05:15:57 PDT
(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.
Comment 8 Zoltan Horvath 2012-04-13 04:18:34 PDT
(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).
Comment 9 noel gordon 2012-04-14 04:43:57 PDT
(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.