Bug 83132 – [Qt] Separate image encoding from dataURL construction

• Component: Canvas
• Status: RESOLVED FIXED

[noel gordon]

[Qt] Separate image encoding from dataURL construction

[noel gordon]

Created attachment 135534 [details]
Patch

[Simon Hausmann]

Comment on attachment 135534 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:383
> +        compressionQuality = static_cast<int>(*quality * 100 + 0.5);

Why not use ceil or round here?

[noel gordon]

> Why not use ceil or round here?

Left it as it was before, and also matches the other webkit ports.

[Noam Rosenthal]

Comment on attachment 135534 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:382
> +    if (quality && *quality >= 0.0 && *quality <= 1.0)

Use 0 and 1, not 0.0 and 1.0

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:406
> +    if (!encodeImage(m_data.m_pixmap, mimeType.substring(sizeof "image"), quality, data))
> +        return "data:,";

Any reason why not to use QMimeDatabase and QMimeType::suffixes()?

[noel gordon]

(In reply to comment #4)
> (From update of attachment 135534 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135534&action=review
> 
> > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:382
> > +    if (quality && *quality >= 0.0 && *quality <= 1.0)
> 
> Use 0 and 1, not 0.0 and 1.0

Possible but again, just re-factoring change here, and it matches the other webkit ports, so I'd prefer to leave it as is.

> > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:406
> > +    if (!encodeImage(m_data.m_pixmap, mimeType.substring(sizeof "image"), quality, data))
> > +        return "data:,";
> 
> Any reason why not to use QMimeDatabase and QMimeType::suffixes()?

Interesting, though not sure how they'd help.  At routine entry we have:

  ASSERT(MIMETypeRegistry::isSupportedImageMIMETypeForEncoding(mimeType));

so the mimeType is already canonical.

[noel gordon]

Thanks for the r+ Noam. I have some questions.

I believe that QPixmap.save() is not safe to use off the main thread. Is that correct?

Since the mimeType is known canonical via the ASSERT, why are the following lines needed?

   if (!mimeType.startsWith("image/"))
       return "data:,";

Is there a reason to prefer the QT toBase64 encoder over the base64 encoder provided by WebCore?

   return "data:" + mimeType + ";base64," + data.toBase64().data();

[WebKit Review Bot]

Comment on attachment 135534 [details]
Patch

Clearing flags on attachment: 135534

Committed r113805: <http://trac.webkit.org/changeset/113805>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #6)
> Thanks for the r+ Noam. I have some questions.
>
> Since the mimeType is known canonical via the ASSERT, why are the following lines needed?
> 
>    if (!mimeType.startsWith("image/"))
>        return "data:,";

Filed bug 83746 about that if you'd like to review.