Bug 29279 – [Qt] Use RGB16 format for images on Symbian platform

• Component: Platform
• Status: RESOLVED INVALID

[Chang Shu]

To reduce the memory footprint on Qt Symbian, rgb565 format(16 bit) can be used for displaying images.

[Chang Shu]

Created attachment 39613 [details]
implement support for rgb565 format in ImageDecoderQt.cpp

[Eric Seidel (no email)]

I don't understand what this patch does.  But I've CC'd peter who hopefully does.

Also, single-line ifs do not get { } per the style guideline.

[Peter Kasting]

(In reply to comment #2)
> I don't understand what this patch does.  But I've CC'd peter who hopefully
> does.

Clearly, it calls some Qt API.  That's all I know; I don't know the Qt APIs.

[Eric Seidel (no email)]

Oh, I totally didn't realize that m_image was a QImage, but that makes sense.  No clue if this is correct then.  A Qt reviewer would have to comment.  This patch is wrong though due to the style as is.  So it will need at least one more round regardless.

[Chang Shu]

Created attachment 39693 [details]
modified with coding style change

[Chang Shu]

Can anyone from Qt review the code? Thanks very much, -- Chang

[Ariya Hidayat]

Comment on attachment 39693 [details]
modified with coding style change


> +        Images with no alpha channel will be converted to 16bit(rgb565) format in Qt Symbian.
> +        This reduces the memory footprint for displaying images.
> +        https://bugs.webkit.org/show_bug.cgi?id=29279

Hardcoding the image format sounds like a wrong thing to do. What about having a setting for that (QWebSettings)?

Also, what if the display depth is 24-bit or 32-bit? We save memory footprint with the patch, but we slow down the painting (even without alpha-blending) because of 16-to-32 pixel handling.

[Chang Shu]

Created attachment 40065 [details]
use qwebsettings to control whether or not use 16-bit image

[Ariya Hidayat]

I think a proper entry in QWebSettings, e.g. accepting QImage::Format, would be better (from API point of view).

[Chang Shu]

Created attachment 40138 [details]
patch with different QWebSettings API

[Chang Shu]

Hi, Ariya,

Even I have submitted the new patch that uses QImage::Format in API, I am re-thinking this and feel there is some disadvantage of exposing QImage::Format. First, QImage::Format is internal definitions and may change more frequently than public interface. Second, it has many more formats that we don't really support. And what we really care is the bit-length of the format not other details. So I plan to use a new enum that only has rgb16 and rgb32. Would you let me know what your thought on this? Thanks,

Chang

(In reply to comment #9)
> I think a proper entry in QWebSettings, e.g. accepting QImage::Format, would be
> better (from API point of view).

[Holger Freyther]

ImageDecoder should not know about anything from WebKit. Then this method is quite hot and doing a color conversion is not cool for performance.

What is preventing us to decode the image to max RGB565, e.g. by changing QImageReader. The other question is what is QPixmap on symbian, a QImage with RGB565?

Last, do you notice that you consume more memory in some cases?

[Ariya Hidayat]

> First, QImage::Format is internal definitions and may change
> more frequently than public interface. Second, it has many more formats that we
> don't really support. And what we really care is the bit-length of the format
> not other details. So I plan to use a new enum that only has rgb16 and rgb32.
> Would you let me know what your thought on this? Thanks,

It's a public API, it is not "internal" at all.

Another concern: what about 5-5-5 in some other platforms (not Symbian)?

[Holger Freyther]

 Some more comments why I think it is bad:

From webkit.org point of view:
   - WebCore/* should not know about WebKit/*

From performance point of view:
   - For certain things using 16bit per pixel is more than the original image used. This can be the case for indexed gifs, PNG that are only 8 bit.

   - There is certainly a speed benefit when the image is in the screen depth as it makes blitting them faster.



The problem is that the API user will not get this right. So in the worse case he increases memory usage and  reduces performance. A better API would say "optimize" for speed, "optimize" for memory. And in both cases  QImage reader should do the right thing for decoding.

[Chang Shu]

(In reply to comment #13)
> > First, QImage::Format is internal definitions and may change
> > more frequently than public interface. Second, it has many more formats that we
> > don't really support. And what we really care is the bit-length of the format
> > not other details. So I plan to use a new enum that only has rgb16 and rgb32.
> > Would you let me know what your thought on this? Thanks,
> 
> It's a public API, it is not "internal" at all.
> 
> Another concern: what about 5-5-5 in some other platforms (not Symbian)?

The conversion is triggered only if
1. no alpha channels
2. the original format is RGB32
3. new format is RGB16

With the current patch, no code changes the default setting to RGB16. Even browser client from other platform starts to use this API, whether internally 5-5-5 or 5-6-5 is used is transparent to the client. I should probably change the title of this bug to "Using RGB16..." to make it less confusing.

[Chang Shu]

(In reply to comment #14)

Thanks for the review. Please see my reply below.

>  Some more comments why I think it is bad:
> 
> From webkit.org point of view:
>    - WebCore/* should not know about WebKit/*

I agree this is valid concern from design. However, the current WebCore/platform/qt code already includes code from WebKit/qt/*, see for example,
WebCore/platform/graphics/qt/ImageQt.cpp:#include "qwebsettings.h". We can probably improve the code some time later.

> 
> From performance point of view:
>    - For certain things using 16bit per pixel is more than the original image
> used. This can be the case for indexed gifs, PNG that are only 8 bit.

The patch checks the original format of the image. The conversion makes sure it converts 32-bit to 16-bit.

> 
>    - There is certainly a speed benefit when the image is in the screen depth
> as it makes blitting them faster.

I totally agree. Fortunately, based on testing, the speed impact is much less while the memory reduction is significant. Note this patch only provides the API and implementation. It does not force the conversion by default.

> 
> 
> 
> The problem is that the API user will not get this right. So in the worse case
> he increases memory usage and  reduces performance. A better API would say
> "optimize" for speed, "optimize" for memory. And in both cases  QImage reader
> should do the right thing for decoding.

We can say the default behavior is "optimized" for speed and if the setting is changed to RGB16, it is "optimized" for memory. Any suggestion on naming the API function is appreciated.

[Holger Freyther]

(In reply to comment #15)
> (In reply to comment #13)
> > > First, QImage::Format is internal definitions and may change
> > > more frequently than public interface. Second, it has many more formats that we
> > > don't really support. And what we really care is the bit-length of the format
> > > not other details. So I plan to use a new enum that only has rgb16 and rgb32.
> > > Would you let me know what your thought on this? Thanks,
> > 
> > It's a public API, it is not "internal" at all.
> > 
> > Another concern: what about 5-5-5 in some other platforms (not Symbian)?
> 
> The conversion is triggered only if
> 1. no alpha channels
> 2. the original format is RGB32
> 3. new format is RGB16

Ah sorry, I didn't attempt to fully parse the code.


> 
> With the current patch, no code changes the default setting to RGB16. Even
> browser client from other platform starts to use this API, whether internally
> 5-5-5 or 5-6-5 is used is transparent to the client. I should probably change
> the title of this bug to "Using RGB16..." to make it less confusing.

Well the point is that this is a decision that the API user should not take as
he has no idea about the consequences on the platform. On X11 this will kill
performance even more.

Two factors for speed and memory usage: Decode the image to what you want
instead of doing color conversion afterwards (in QImageReader/QImageIO), limit
the maximum size of images (see WebCore decoders).

And there is an easy way to convince me, I have created benchmarks and if less
memory in these benchmarks is allocated and they perform faster I'm open to any
change. So far I think the fix should be in QImageReader.

[Holger Freyther]

(In reply to comment #16)
> (In reply to comment #14)

> I agree this is valid concern from design. However, the current
> WebCore/platform/qt code already includes code from WebKit/qt/*, see for
> example,
> WebCore/platform/graphics/qt/ImageQt.cpp:#include "qwebsettings.h". We can
> probably improve the code some time later.

Well, this is not a reason to do more harm.


> I totally agree. Fortunately, based on testing, the speed impact is much less
> while the memory reduction is significant. Note this patch only provides the
> API and implementation. It does not force the conversion by default.

The great thing is that we can measure this now and share results: http://trac.webkit.org/wiki/QtWebKitPerformanceWork. If you are with Nokia I can probably give you access to my existing test database. We can both look into loading, cycling speed and memory usage.

Besides that the default depth should be handled by QImageReader and not in WebCore and you need to rebase now as a major rework of the image handling was just landed.

[Chang Shu]

(In reply to comment #17)

Thanks for the quick reply.

> (In reply to comment #15)
> 
> Well the point is that this is a decision that the API user should not take as
> he has no idea about the consequences on the platform. On X11 this will kill
> performance even more.
> 
> Two factors for speed and memory usage: Decode the image to what you want
> instead of doing color conversion afterwards (in QImageReader/QImageIO), limit
> the maximum size of images (see WebCore decoders).
> 
> And there is an easy way to convince me, I have created benchmarks and if less
> memory in these benchmarks is allocated and they perform faster I'm open to any
> change. So far I think the fix should be in QImageReader.

If the change is in QImageReader, other Qt applications than browser will be affected. Some other Qt guys said no on this.

[Chang Shu]

(In reply to comment #18)
> 
> Well, this is not a reason to do more harm.

Yes, yes.  :) But I have to work on what I have now. Good that the change is still limited to qwebsetting which can be re-designed by Qt.
> 
> 
> 
> The great thing is that we can measure this now and share results:
> http://trac.webkit.org/wiki/QtWebKitPerformanceWork. If you are with Nokia I
> can probably give you access to my existing test database. We can both look
> into loading, cycling speed and memory usage.

Yes, I will talk to you through email. Thanks

> 
> Besides that the default depth should be handled by QImageReader and not in
> WebCore and you need to rebase now as a major rework of the image handling was
> just landed.

The concern was whether other Qt applications want this feature.

[Simon Hausmann]

Comment on attachment 40138 [details]
patch with different QWebSettings API


>          LocalContentCanAccessRemoteUrls,
>          SessionStorageEnabled,
> -        DnsPrefetchEnabled
> +        DnsPrefetchEnabled,

The trailing comma shouldn't be there :)

>      };
>      enum WebGraphic {
>          MissingImageGraphic,
> @@ -134,6 +134,9 @@ public:
>  
>      static void enablePersistentStorage(const QString& path = QString());
>  
> +    void setImageFormat(QImage::Format format);
> +    QImage::Format imageFormat();
> +

If this is a regular member function of QWebSettings, then it must also be possible to 
set the image format per-page. However in the implementation we can only use QWebSettings::globalSettings(), we
do not have access to the page. Therefore if we decide to use this API, then we it should be a static function in QWebSettings.


I am however concerned that we need such an API in the first place. Is there any way we can make a smart
decision ourselves? Do we really have to bother the application programmer with this?

And most importantly: Is this optimization still necessary after Holger's changes from #27538 ?

[Holger Freyther]

Created attachment 41097 [details]
Memory with RGB16 instead of RGB32

[Holger Freyther]

Created attachment 41098 [details]
Memory usage before the QImageReader change

[Holger Freyther]

Created attachment 41099 [details]
API for QImageReader...

My metric skills are not so good but this looks like a 1.6MB memory saving on images from my benchmark. Can you confirm that you see similiar numbers?

[Chang Shu]

(In reply to comment #24)
> Created an attachment (id=41099) [details]
> API for QImageReader...
> 
> My metric skills are not so good but this looks like a 1.6MB memory saving on
> images from my benchmark. Can you confirm that you see similiar numbers?

I tried Holger's code locally. First, I tried it on Symbian OS with local webkit database. However, since our local webkit does not have the latest image re-work, the code didn't work after I simply call setPreferredFormat in setData. I need more time to do the debugging. I also tried it on Qt Linux. I applied the patch on Qt4.5. I see the patch is executed successfully. However, there is a call to buffer->asNewNativeImage() inside function ImageSource::createFrameAtIndex(). This converts 16bit image back to 32bit image as the latter is the native format. And this explains why I didn't see any improvement through memory monitor tool.
Good that in Qt NSP version, the conversion to native format won't happen automatically(in QPixmap::fromImage()). Since NSP does not work on Linux, I have to setup a new work environment to test this.

Saying all the above, I am concerned about the availability of Holger's code. It seems too late to wait for 4.7. I am wondering if we can do this convert-to-preferred-format logic in ImageDecoderQt::internalHandleCurrentImage() for now. Besides, even put the logic in Qt, we still have the headache of how to trigger this: whether through qwebsettings api, or by ifdef(SYMBIAN) or for all Qt platform.

[Holger Freyther]

(In reply to comment #25)

> I tried Holger's code locally. First, I tried it on Symbian OS with local
> webkit database. However, since our local webkit does not have the latest image
> re-work, the code didn't work after I simply call setPreferredFormat in
> setData. I need more time to do the debugging. I also tried it on Qt Linux. I
> applied the patch on Qt4.5. I see the patch is executed successfully. However,
> there is a call to buffer->asNewNativeImage() inside function
> ImageSource::createFrameAtIndex(). This converts 16bit image back to 32bit
> image as the latter is the native format. And this explains why I didn't see
> any improvement through memory monitor tool.
> Good that in Qt NSP version, the conversion to native format won't happen
> automatically(in QPixmap::fromImage()). Since NSP does not work on Linux, I
> have to setup a new work environment to test this.

Note, my 1.7memory saving is not taking the QImage->QPixmap into account. In my benchmark asNewNativeImage is not called at all. I have created bug #30211 to make use of QImage instead of QPixmap for painting but I have not yet benchmarked it.


> 
> Saying all the above, I am concerned about the availability of Holger's code.
> It seems too late to wait for 4.7. I am wondering if we can do this
> convert-to-preferred-format logic in
> ImageDecoderQt::internalHandleCurrentImage() for now.

No. We are fixing things where they need to be fixed. I have no idea about constraints but you will probably have more than one patch to Qt when doing a release/deployment. Just add the API after Qt has agreed that it will be accepted.


Currently I'm not really convinced that the 1.7MB is a big difference, do you have other data showing a bigger saving?

[Chang Shu]

(In reply to comment #26)
> Note, my 1.7memory saving is not taking the QImage->QPixmap into account. In my
> benchmark asNewNativeImage is not called at all. I have created bug #30211 to
> make use of QImage instead of QPixmap for painting but I have not yet
> benchmarked it.

Can you provide some details about your test case:
1. what's the size of your image(s)?
2. Without the image, what's the memory usage?

But anyway, we can calculate the memory saving = w*h*4/2. For example, a 800x600 image saves 0.96MB.

> No. We are fixing things where they need to be fixed. I have no idea about
> constraints but you will probably have more than one patch to Qt when doing a
> release/deployment. Just add the API after Qt has agreed that it will be
> accepted.

I totally agree adding API as early as possible is great idea. This doesn't prevent us doing the implementation at webkit level for now. Otherwise, we have to wait for mid next year to get the job done.

> Currently I'm not really convinced that the 1.7MB is a big difference, do you
> have other data showing a bigger saving?

I won't think the memory saving will the different as long as we find out three figures:
the "baseline": memory consumption with no image; memory consumption with image using this 16bit conversion and without this conversion. Based on the calculation, I feel it's worthwhile doing it on mobile platforms. Maybe a #ifdef MOBILE is a good way to go.

[Holger Freyther]

(In reply to comment #27)

> Can you provide some details about your test case:
> 1. what's the size of your image(s)?
> 2. Without the image, what's the memory usage?
> 
> But anyway, we can calculate the memory saving = w*h*4/2. For example, a
> 800x600 image saves 0.96MB.

I'm using the real web with row01 to row39 and I captured images that got decoded during rendering on a 1024x768 viewport. I have measured the memory usage on the reductions/image_cycling in the same tree.

  - http://gitorious.org/qtwebkit/performance/blobs/master/common/common_init.h



> 
> > No. We are fixing things where they need to be fixed. I have no idea about
> > constraints but you will probably have more than one patch to Qt when doing a
> > release/deployment. Just add the API after Qt has agreed that it will be
> > accepted.
> 
> I totally agree adding API as early as possible is great idea. This doesn't
> prevent us doing the implementation at webkit level for now. Otherwise, we have
> to wait for mid next year to get the job done.

One thing after another. Currently this will only add a negative performance  impact on converting to 16bit in Qt/ImageDecoder and converting back to 32bit in QPixmap::fromImage.

So let us focus on:

1.) Showing that the color conversion from RGB32 to RGB16 is actually saving significant amount of memory and then to look at the performance impact of that change.

2.) Look at decoding gif's to 8bit or below...





> 
> > Currently I'm not really convinced that the 1.7MB is a big difference, do you
> > have other data showing a bigger saving?
> 
> I won't think the memory saving will the different as long as we find out three
> figures:
> the "baseline": memory consumption with no image; memory consumption with image
> using this 16bit conversion and without this conversion.

For the baseline. Looking at the mirrored sites, do you think anything major is missing? Should we have a pure top50 data set as well. I highly encourage you start to look into the mirror tool and attempt to verify my benchmark.

[Benjamin Poulain]

Closing as invalid.

I might be wrong but it looks like:
-images are always decoded to QPixmap
-QPixmap have the correct color depth (tested on Maemo)

So 32 bits images are kept only into m_frameBufferCache of ImageDecoder. It might make sense to keep ARGB image in 32 bits, but that is a problem of Qt which is currently being investigated by the graphics team.