Bug 88785 – [Qt] Change NativeImagePtr from QPixmap* to QImage*

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Zoltan Horvath]

Since we use raster engine there is no difference between QImage and QPixmap classes.
I think it would clarify the things if we could use only the QImage class and get rid of QPixmap classes (it's possible almost everywhere). 

Additionally QImage objects are thread safe, so it can be useful for us later for cross-thread optimizations.
I have a work in progress patch on this topic, if you agree with my points, I can continue this work.

[Allan Sandfeld Jensen]

Additionally moving to QImage only should make it safe to use either native or raster engine.

[Zoltan Horvath]

Created attachment 147283 [details]
proposed patch

[Zoltan Horvath]

We should also talk about where to use NativeImagePtr and where to use the class name, because it's not clear. What is your opinion about use the NativeImagePtr typedef everywhere under WebCore/platform/graphics, and use the real class name outside of that directory?

[WebKit Review Bot]

Attachment 147283 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/qt/Api/qwebframe.cpp:1767:  Declaration has space between type name and * in QImage *pix  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/GraphicsContext.h:504:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/qt/StillImageQt.h:59:  The parameter name "image" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/qt/StillImageQt.h:60:  The parameter name "image" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/qt/ImageBufferDataQt.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:150:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebCore/platform/graphics/qt/TransparencyLayer.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 7 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Horvath]

I fixed the stile issues. I'm not going to reupload the patch until it gets review, since it will have problems with: Source/WebKit/qt/Api/qwebframe.cpp:1767:  Declaration has space between type name and * in QImage *pix  [whitespace/declaration] [3]
I will file a bug aganst check-webkit-style.

[Viatcheslav Ostapenko]

Comment on attachment 147283 [details]
proposed patch

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

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:490
>          nativeImage = QImage::fromData(reinterpret_cast<const uchar*>(image->data()->data()), image->data()->size()).convertToFormat(QImage::Format_ARGB32);

Why not Format_ARGB32_Premultiplied.
Also, do use ARGB formats also for images without alpha?
And, it would be good to define default image formats in single place for easy redefine if other formats required for small devices.

[Viatcheslav Ostapenko]

(In reply to comment #0)
> Since we use raster engine there is no difference between QImage and QPixmap classes.
> I think it would clarify the things if we could use only the QImage class and get rid of QPixmap classes (it's possible almost everywhere). 
> 
> Additionally QImage objects are thread safe, so it can be useful for us later for cross-thread optimizations.
> I have a work in progress patch on this topic, if you agree with my points, I can continue this work.

Sorry for late comment. Didn't see this bug. Thanks Laszlo for forwarding.
I don't think we want to get rid of QPixmap. It was still faster with QPixmaps on some engines (if there was enough graphics memory) on webkit1. 
Also, even with webkit2 QPixmaps might help, with direct opengl surface rendering will be implemented.
We have also changed QPixmaps to QImages on our branch, but implemented it switchable. I'll attach patch for reference.

[Noam Rosenthal]

> I don't think we want to get rid of QPixmap. It was still faster with QPixmaps on some engines (if there was enough graphics memory) on webkit1. 
With Qt5? Which engines (if you can share that info)?

> Also, even with webkit2 QPixmaps might help, with direct opengl surface rendering will be implemented.
We're not going to use QPixmaps for that, but rather a special-case class that does just that.

[Viatcheslav Ostapenko]

Created attachment 147325 [details]
Commit form internal branch for qpixmap2qimage.

[Viatcheslav Ostapenko]

(In reply to comment #8)
> > I don't think we want to get rid of QPixmap. It was still faster with QPixmaps on some engines (if there was enough graphics memory) on webkit1. 
> With Qt5? Which engines (if you can share that info)?

In Qt4 on opengl engine QPixmap had image uploaded to texture for faster rendering and also backed with framebuffer (if GL engine supports) for rendering on pixmap. That was in src/opengl/qpixmapdata* . Was it removed?

> > Also, even with webkit2 QPixmaps might help, with direct opengl surface rendering will be implemented.
> We're not going to use QPixmaps for that, but rather a special-case class that does just that.

No, I don't mean painting on QPixmap.
It is possible to create QPainter form QGLFramebufferObject and in this case qpixmap backed by texture would paint faster, than qimage.

[Simon Hausmann]

(In reply to comment #10)
> (In reply to comment #8)
> > > I don't think we want to get rid of QPixmap. It was still faster with QPixmaps on some engines (if there was enough graphics memory) on webkit1. 
> > With Qt5? Which engines (if you can share that info)?
> 
> In Qt4 on opengl engine QPixmap had image uploaded to texture for faster rendering and also backed with framebuffer (if GL engine supports) for rendering on pixmap. That was in src/opengl/qpixmapdata* . Was it removed?

AFAICS in Qt 5 the GL paint-engine is accelerating QPainter::drawPixmap through an internal texture cache, not through a custom pixmap data implementation.

> > > Also, even with webkit2 QPixmaps might help, with direct opengl surface rendering will be implemented.
> > We're not going to use QPixmaps for that, but rather a special-case class that does just that.
> 
> No, I don't mean painting on QPixmap.
> It is possible to create QPainter form QGLFramebufferObject and in this case qpixmap backed by texture would paint faster, than qimage.

The question is: Where in WebKit would we end up _actually_ using any "features" of QPixmap beyond the raster backing?

Certainly not in WebKit2 (where GraphicsSurface is our new QPixmap :), so only WebKit1 remains.

And that's where we have to make a decision I think, what type of rendering to we want to support, encourage or recommend for WebKit1 on Qt 5. A hard way would be to say software-only with raster engine and software backed texture mapper.

[Viatcheslav Ostapenko]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> AFAICS in Qt 5 the GL paint-engine is accelerating QPainter::drawPixmap through an internal texture cache, not through a custom pixmap data implementation.

So, QPixmap painting is still accelerated, as I understand?

> > > > Also, even with webkit2 QPixmaps might help, with direct opengl surface rendering will be implemented.
> > > We're not going to use QPixmaps for that, but rather a special-case class that does just that.
> > 
> > No, I don't mean painting on QPixmap.
> > It is possible to create QPainter form QGLFramebufferObject and in this case qpixmap backed by texture would paint faster, than qimage.
> 
> The question is: Where in WebKit would we end up _actually_ using any "features" of QPixmap beyond the raster backing?

From my experiments with openvg this is still faster then pure raster if there is enough graphics RAM for qpixmap back buffers.

> Certainly not in WebKit2 (where GraphicsSurface is our new QPixmap :), so only WebKit1 remains.

Do we have plans to implement GL drawing on GraphicsSurface?
As I understand, we'll have to create QPainter (webkit context) from GraphicsSurface?
In this case, IMHO, texture backed pixmaps would paint faster.

> And that's where we have to make a decision I think, what type of rendering to we want to support, encourage or recommend for WebKit1 on Qt 5. A hard way would be to say software-only with raster engine and software backed texture mapper.

IMHO, for hybrid applications simple webkit1 webview is better.
I think webkit1 webview/pageview similar to QQuickWebView/QQuickWebPage, but with webcore in the same process, will be very usable.

[Noam Rosenthal]

> Do we have plans to implement GL drawing on GraphicsSurface?
Yes, we already do that for WebGL.

> As I understand, we'll have to create QPainter (webkit context) from GraphicsSurface?
Probably the QPainter abstraction is not going to help there.

[Zoltan Horvath]

Created attachment 147343 [details]
proposed patch

I addressed Viatcheslav comments!
The style should be okay now because I fixed the check-webkit-style rules.

[Noam Rosenthal]

Comment on attachment 147343 [details]
proposed patch

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

There are a couple of rebases needed... Otherwise I'm pretty positive about reviewing this. If anyone has "big" objections please add them soon :)

> Source/WebCore/platform/graphics/ImageSource.h:94
> +#define OPAQUE_IMAGE_FORMAT      QImage::Format_RGB888

Shouldn't this be Format_RGB32?

> Source/WebCore/platform/graphics/ImageSource.h:95
> +#define OPAQUE_IMAGE_FORMAT      QImage::Format_RGB888
> +#define TRANSPARENT_IMAGE_FORMAT QImage::Format_ARGB32_Premultiplied

I'd rather this be an enum instead of #defines.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:493
> -    else {
> -        QPixmap* nativePixmap = image->nativeImageForCurrentFrame();
> -        nativeImage = nativePixmap->toImage().convertToFormat(QImage::Format_ARGB32);
> -    }
> +    else
> +        nativeImage = image->nativeImageForCurrentFrame()->convertToFormat(QImage::Format_ARGB32);
> +

Probably needs a rebase.

> Source/WebCore/platform/graphics/qt/TransparencyLayer.h:39
> +

Unnecessary line.

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:407
>  int64_t LayerTreeHostQt::adoptImageBackingStore(Image* image)
>  {
>      if (!image)
>          return InvalidWebLayerID;
> -    QPixmap* pixmap = image->nativeImageForCurrentFrame();
> +    QImage* nativeImage = image->nativeImageForCurrentFrame();
>  
> -    if (!pixmap)
> +    if (!nativeImage)
>          return InvalidWebLayerID;
>  
> -    int64_t key = pixmap->cacheKey();
> +    int64_t key = nativeImage->cacheKey();
>      HashMap<int64_t, int>::iterator it = m_directlyCompositedImageRefCounts.find(key);
>  
>      if (it != m_directlyCompositedImageRefCounts.end()) {

Probably needs a rebase.

[Viatcheslav Ostapenko]

Comment on attachment 147343 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/ImageSource.h:94
>> +#define OPAQUE_IMAGE_FORMAT      QImage::Format_RGB888
> 
> Shouldn't this be Format_RGB32?

Actually, why?
It requires 33% more RAM, but performance improvement was not big when I measured it.

[Noam Rosenthal]

(In reply to comment #16)
> (From update of attachment 147343 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147343&action=review
> 
> >> Source/WebCore/platform/graphics/ImageSource.h:94
> >> +#define OPAQUE_IMAGE_FORMAT      QImage::Format_RGB888
> > 
> > Shouldn't this be Format_RGB32?
> 
> Actually, why?
> It requires 33% more RAM, but performance improvement was not big when I measured it.

I thought there were special optimizations for RGB32, I don't recall using 888 much. But maybe it doesn't matter.

[Allan Sandfeld Jensen]

(In reply to comment #16)
> (From update of attachment 147343 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147343&action=review
> 
> >> Source/WebCore/platform/graphics/ImageSource.h:94
> >> +#define OPAQUE_IMAGE_FORMAT      QImage::Format_RGB888
> > 
> > Shouldn't this be Format_RGB32?
> 
> Actually, why?
> It requires 33% more RAM, but performance improvement was not big when I measured it.

There is a lot of code currently assuming a pixel-size of 4 bytes. If those places are fixed I would be all for allowing 24bit encoding (I have been thinking about doing this myself), but it would probably need a feature-flag since it might be slower on some platforms without native hardware support. 

I would suggest for this patch, to just use RGB32. If you want to enable 24bit, please do so in another patch that solves all the pixel-size problems.

[Viatcheslav Ostapenko]

(In reply to comment #18)
> (In reply to comment #16)
> > (From update of attachment 147343 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=147343&action=review
> > 
> > >> Source/WebCore/platform/graphics/ImageSource.h:94
> > >> +#define OPAQUE_IMAGE_FORMAT      QImage::Format_RGB888
> > > 
> > > Shouldn't this be Format_RGB32?
> > 
> > Actually, why?
> > It requires 33% more RAM, but performance improvement was not big when I measured it.
> 
> There is a lot of code currently assuming a pixel-size of 4 bytes. If those places are fixed I would be all for allowing 24bit encoding (I have been thinking about doing this myself), but it would probably need a feature-flag since it might be slower on some platforms without native hardware support. 
> 
> I would suggest for this patch, to just use RGB32. If you want to enable 24bit, please do so in another patch that solves all the pixel-size problems.

Actually, on internal branch we used 16 bit color for opaque images. The only extra place we changed was image decoders to produce 16 bit output. Worked fine.

[Noam Rosenthal]

> > I would suggest for this patch, to just use RGB32. If you want to enable 24bit, please do so in another patch that solves all the pixel-size problems.
> 
> Actually, on internal branch we used 16 bit color for opaque images. The only extra place we changed was image decoders to produce 16 bit output. Worked fine.

We can do that at a different patch. For now let's focus on switching from QPixmap to QImage.

[Zoltan Horvath]

(In reply to comment #20)
> We can do that at a different patch. For now let's focus on switching from QPixmap to QImage.

I'm going to update the patch today.

[Zoltan Horvath]

Created attachment 152222 [details]
proposed patch

[Zoltan Horvath]

(In reply to comment #15)
> Shouldn't this be Format_RGB32?

Done.
 
> > Source/WebCore/platform/graphics/ImageSource.h:95
> > +#define OPAQUE_IMAGE_FORMAT      QImage::Format_RGB888
> > +#define TRANSPARENT_IMAGE_FORMAT QImage::Format_ARGB32_Premultiplied
> 
> I'd rather this be an enum instead of #defines.

QImage's constructor takes a QImage::Format, so how can I transform them to an enum then?

> > Source/WebCore/platform/graphics/qt/TransparencyLayer.h:39
> > +
> 
> Unnecessary line.

Removed
 
> Probably needs a rebase.

Rebased and tested.

[Simon Hausmann]

Comment on attachment 152222 [details]
proposed patch

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

Haven't taken the time yet too look at the changes in detail, but I agree about the direction. One small nitpick I noticed below.

> Source/WebCore/platform/graphics/qt/ImageDecoderQt.cpp:277
>      QImage img(reinterpret_cast<uchar*>(m_bytes), m_size.width(), m_size.height(), sizeof(PixelData) * m_size.width(), format);
>  
> -    return new QPixmap(QPixmap::fromImage(img));
> +    return new QImage(img);

Hmmm, if it's guaranteed that the caller keeps the ImageFrame around, then wouldn't it be simpler to just do return new QImage(reinterpret_cast<uchar*>(m_bytes), ...) ?

If that's not guaranteed, then we probably need a call to copy() to make sure that the data is properly detached.

[Noam Rosenthal]

Comment on attachment 152222 [details]
proposed patch

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

TRANSPARENT_

> Source/WebCore/platform/graphics/ImageSource.h:95
> +#define OPAQUE_IMAGE_FORMAT      QImage::Format_RGB32
> +#define TRANSPARENT_IMAGE_FORMAT QImage::Format_ARGB32_Premultiplied

I still don't like having these defines here.
How about adding a NativeImageQt file, and in it put something like
static QImage::Format NativeImageQt::defaultFormatFor[AlphaEnabled|Opaque]Images()

[Zoltan Horvath]

Created attachment 152276 [details]
proposed patch

[Zoltan Horvath]

(In reply to comment #24)
> Hmmm, if it's guaranteed that the caller keeps the ImageFrame around, then wouldn't it be simpler to just do return new QImage(reinterpret_cast<uchar*>(m_bytes), ...) ?
> 
> If that's not guaranteed, then we probably need a call to copy() to make sure that the data is properly detached.

It is guaranteed. I modified the return.(In reply to comment #25)
> (From update of attachment 152222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152222&action=review
> 
> TRANSPARENT_
> 
> > Source/WebCore/platform/graphics/ImageSource.h:95
> > +#define OPAQUE_IMAGE_FORMAT      QImage::Format_RGB32
> > +#define TRANSPARENT_IMAGE_FORMAT QImage::Format_ARGB32_Premultiplied
> 
> I still don't like having these defines here.
> How about adding a NativeImageQt file, and in it put something like
> static QImage::Format NativeImageQt::defaultFormatFor[AlphaEnabled|Opaque]Images()

Done. There are a few places where we can also use the newly added functions, but I would do that in a separate patch.

[Noam Rosenthal]

Comment on attachment 152276 [details]
proposed patch

This looks good to me, but let's wait to see if Simon wants to have a look as well.

[Simon Hausmann]

Comment on attachment 152276 [details]
proposed patch

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

r=me

Land with care and watch the bots ;)

> Source/WebKit/qt/Api/qwebframe.cpp:1977
> -    return d->pixmap;
> +    return QPixmap::fromImage(d->image);

Is this a good move? QPixmap::fromImage will do a conversion every time, making the pixmap() accessor a lot slower on repeated calls. Wouldn't it be simpler to leave the pixmap as member? (under the assumption that we're not changing the API)

Alternatively the conversion from QImage to QPixmap can be made _very_ cheap if the QImage has the same format, so another option would be for example to make the 'QImage image' member here hold a QImage respecting QPixmap::defaultDepth().

[Zoltan Horvath]

Committed r122720: <http://trac.webkit.org/changeset/122720>