DevHeads.net

Review Request: add kimgio WebP image format plugin

Review request for kdelibs.

Description
This patch adds support for the new Google invented WebP image format. See <a href="https://developers.google.com/speed/webp/?hl=ru" title="https://developers.google.com/speed/webp/?hl=ru">https://developers.google.com/speed/webp/?hl=ru</a>

The patch is missing a cmake rule to make it optional, though, but I sent a mail to KDE-core list in the hope someone knows...

This addresses bug 267365.
<a href="http://bugs.kde.org/show_bug.cgi?id=267365" title="http://bugs.kde.org/show_bug.cgi?id=267365">http://bugs.kde.org/show_bug.cgi?id=267365</a>

Diffs
kimgio/webp.cpp PRE-CREATION
kimgio/webp.desktop PRE-CREATION
kimgio/webp.h PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/106300/diff/" title="http://git.reviewboard.kde.org/r/106300/diff/">http://git.reviewboard.kde.org/r/106300/diff/</a>

Testing
some KDE apps, including read/write with a modified kolourpaint (to be able to change the quality)

Thanks,

Martin Koller

Comments

Re: Review Request: add kimgio WebP image format plugin

By Martin Koller at 09/01/2012 - 17:23

(Updated Sept. 1, 2012, 9:23 p.m.)

Review request for kdelibs.

Changes
changes as per comments

Description
This patch adds support for the new Google invented WebP image format. See <a href="https://developers.google.com/speed/webp/?hl=ru" title="https://developers.google.com/speed/webp/?hl=ru">https://developers.google.com/speed/webp/?hl=ru</a>

The patch is missing a cmake rule to make it optional, though, but I sent a mail to KDE-core list in the hope someone knows...

This addresses bug 267365.
<a href="http://bugs.kde.org/show_bug.cgi?id=267365" title="http://bugs.kde.org/show_bug.cgi?id=267365">http://bugs.kde.org/show_bug.cgi?id=267365</a>

Diffs (updated)
kimgio/CMakeLists.txt 26329c0
kimgio/webp.cpp PRE-CREATION
kimgio/webp.desktop PRE-CREATION
kimgio/webp.h PRE-CREATION
mimetypes/kde.xml a82b87c

Diff: <a href="http://git.reviewboard.kde.org/r/106300/diff/" title="http://git.reviewboard.kde.org/r/106300/diff/">http://git.reviewboard.kde.org/r/106300/diff/</a>

Testing
some KDE apps, including read/write with a modified kolourpaint (to be able to change the quality)

Thanks,

Martin Koller

Re: Review Request 106300: add kimgio WebP image format plugin

By Martin Koller at 05/09/2013 - 14:20

(Updated May 9, 2013, 6:20 p.m.)

Review request for kdelibs.

Changes
added answers to comments and added "Size" option

Description (updated)
This patch adds support for the new Google invented WebP image format. See <a href="https://developers.google.com/speed/webp/?hl=ru" title="https://developers.google.com/speed/webp/?hl=ru">https://developers.google.com/speed/webp/?hl=ru</a>

This addresses bug 267365.
<a href="http://bugs.kde.org/show_bug.cgi?id=267365" title="http://bugs.kde.org/show_bug.cgi?id=267365">http://bugs.kde.org/show_bug.cgi?id=267365</a>

Diffs (updated)
kimgio/CMakeLists.txt 26329c0
kimgio/webp.cpp PRE-CREATION
kimgio/webp.desktop PRE-CREATION
kimgio/webp.h PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/106300/diff/" title="http://git.reviewboard.kde.org/r/106300/diff/">http://git.reviewboard.kde.org/r/106300/diff/</a>

Testing
some KDE apps, including read/write with a modified kolourpaint (to be able to change the quality)

Thanks,

Martin Koller

Re: Review Request 106300: add kimgio WebP image format plugin

By Commit Hook at 05/18/2013 - 08:44

(Updated May 18, 2013, 12:44 p.m.)

Status
This change has been marked as submitted.

Review request for kdelibs.

Description
This patch adds support for the new Google invented WebP image format. See <a href="https://developers.google.com/speed/webp/?hl=ru" title="https://developers.google.com/speed/webp/?hl=ru">https://developers.google.com/speed/webp/?hl=ru</a>

This addresses bug 267365.
<a href="http://bugs.kde.org/show_bug.cgi?id=267365" title="http://bugs.kde.org/show_bug.cgi?id=267365">http://bugs.kde.org/show_bug.cgi?id=267365</a>

Diffs
kimgio/CMakeLists.txt 26329c0
kimgio/webp.cpp PRE-CREATION
kimgio/webp.desktop PRE-CREATION
kimgio/webp.h PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/106300/diff/" title="http://git.reviewboard.kde.org/r/106300/diff/">http://git.reviewboard.kde.org/r/106300/diff/</a>

Testing
some KDE apps, including read/write with a modified kolourpaint (to be able to change the quality)

Thanks,

Martin Koller

Re: Review Request 106300: add kimgio WebP image format plugin

By Martin Koller at 05/26/2013 - 08:12

(Updated May 26, 2013, 12:12 p.m.)

Status
This change has been marked as submitted.

Review request for kdelibs.

Description
This patch adds support for the new Google invented WebP image format. See <a href="https://developers.google.com/speed/webp/?hl=ru" title="https://developers.google.com/speed/webp/?hl=ru">https://developers.google.com/speed/webp/?hl=ru</a>

This addresses bug 267365.
<a href="http://bugs.kde.org/show_bug.cgi?id=267365" title="http://bugs.kde.org/show_bug.cgi?id=267365">http://bugs.kde.org/show_bug.cgi?id=267365</a>

Diffs
kimgio/CMakeLists.txt 26329c0
kimgio/webp.cpp PRE-CREATION
kimgio/webp.desktop PRE-CREATION
kimgio/webp.h PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/106300/diff/" title="http://git.reviewboard.kde.org/r/106300/diff/">http://git.reviewboard.kde.org/r/106300/diff/</a>

Testing
some KDE apps, including read/write with a modified kolourpaint (to be able to change the quality)

Thanks,

Martin Koller

Re: Review Request 106300: add kimgio WebP image format plugin

By Commit Hook at 05/18/2013 - 08:44

This review has been submitted with commit df845b06bc440fff5a43a9cdc149e8c88c29bc67 by Martin Koller to branch master.

- Commit Hook

On May 9, 2013, 6:20 p.m., Martin Koller wrote:

Re: Review Request 106300: add kimgio WebP image format plugin

By Michael Reeves at 06/16/2013 - 21:16

See <a href="https://git.reviewboard.kde.org/r/110649/" title="https://git.reviewboard.kde.org/r/110649/">https://git.reviewboard.kde.org/r/110649/</a> for move status.

- Michael

On May 26, 2013, 8:12 a.m., Martin Koller wrote:

Re: Review Request 106300: add kimgio WebP image format plugin

By Christoph Feck at 06/15/2013 - 21:13

This still is only in kolourpaint, which is unfortunate. Bug 267365 is a KDE general feature request. People who do not install kolourpaint will be left out.

Additionally, having it installed will make it work with other Qt programs (Gwenview etc.), but not the thumbnailer. I cannot add the mime type to the thumbnailer, if the plugin is only in kolourpaint.

AFAIK, it was decided to move it to kde-runtime.

- Christoph

On May 26, 2013, 12:12 p.m., Martin Koller wrote:

Re: Review Request 106300: add kimgio WebP image format plugin

By Martin Koller at 06/16/2013 - 08:21

On Sunday 16 June 2013 01:13:13 Christoph Feck wrote:
I created a new review request for kde-runtime: <a href="http://git.reviewboard.kde.org/r/110649/" title="http://git.reviewboard.kde.org/r/110649/">http://git.reviewboard.kde.org/r/110649/</a>
I hopefully solved the last open comment from Fredrik Höglund today, so I'm waiting now
for the final "ship it" and hope that this move from kolourpaint to kde-runtime isn't
now too late for 4.11

Re: Review Request 106300: add kimgio WebP image format plugin

By Albert Astals Cid at 05/22/2013 - 18:15

If you addressed Allan's comment i'd sincerely put it where it belongs, which is obviously not kolourpaint :D

- Albert

On May 18, 2013, 12:44 p.m., Martin Koller wrote:

Re: Review Request 106300: add kimgio WebP image format plugin

By Martin Koller at 05/23/2013 - 03:47

On Wednesday 22 May 2013 22:15:56 Albert Astals Cid wrote:
Where does it belong to, as kdelibs is feature-frozen ?

Re: Review Request 106300: add kimgio WebP image format plugin

By Albert Astals Cid at 05/24/2013 - 08:46

El Dijous, 23 de maig de 2013, a les 09:47:36, Martin Koller va escriure:
To kde-runtime.

There's no reason a image format plugin should be in kdelibs anyway since kde-
runtime is a mandatory requirement for running any kdelibs based app.

Cheers,
Albert

Re: Review Request 106300: add kimgio WebP image format plugin

By Christoph Feck at 05/21/2013 - 17:34

Just noticed this has been committed to kolourpaint instead of kdelibs. Did I miss a discussion about this change?

- Christoph

On May 18, 2013, 12:44 p.m., Martin Koller wrote:

Re: Review Request 106300: add kimgio WebP image format plugin

By Martin Koller at 05/22/2013 - 02:40

On Tuesday 21 May 2013 21:34:23 Christoph Feck wrote:
Probably. I sent again an email to kde-core-devel on May 11th, which was ignored, so I thought nobody cared.
As the deadline for 4.11 approached, I decided to commit this to kolourpaint, which I'm the maintainer of,
to at least have it in 4.11

Re: Review Request 106300: add kimgio WebP image format plugin

By Albert Astals Cid at 04/10/2013 - 18:52

Martin, can you clarify Allan's comments?

- Albert Astals Cid

On Sept. 1, 2012, 9:23 p.m., Martin Koller wrote:

Re: Review Request 106300: add kimgio WebP image format plugin

By Martin Koller at 04/27/2013 - 17:31

done

- Martin

On Sept. 1, 2012, 9:23 p.m., Martin Koller wrote:

Re: Review Request: add kimgio WebP image format plugin

By Allan Sandfeld ... at 09/10/2012 - 18:48

Looks good. I started writing such a plugin myself sometime back, so excuse my rather specific comments.

kimgio/webp.cpp
<http://git.reviewboard.kde.org/r/106300/#comment14924>

This seems needlessly explicit, why not just copy the whole block into QImage?

kimgio/webp.cpp
<http://git.reviewboard.kde.org/r/106300/#comment14925>

Same as for decoding. Why copy it color by color like this, when the formats are identical?

kimgio/webp.cpp
<http://git.reviewboard.kde.org/r/106300/#comment14926>

Not really required, but it would be nice to support the Size option. It is quite often used, and should be available through WebPGetInfo()

kimgio/webp.cpp
<http://git.reviewboard.kde.org/r/106300/#comment14927>

Should it also require "VP8 " in byte 12 to 16, to protect against new unsupported versions?

For some reason I have code that checks the file is atleast 32 bytes long, I think that is because it is what WePGetInfo requires.

- Allan Sandfeld Jensen

On Sept. 1, 2012, 9:23 p.m., Martin Koller wrote:

Re: Review Request 106300: add kimgio WebP image format plugin

By Martin Koller at 04/27/2013 - 16:46

QImage::scanLine docu says "the pixel format depends on the byte order on the underlying platform."
So to be sure to create correct rgba tuples, I explicitely pass the values to qRgba/qRgb

Same answer as above: scanLine data depends on the machines byte order, but WebP has always the same data representation

ok, added

not really. The spec says that the WebP header is 12 bytes and contains what I check.
There is an Extended file format which starts with a ‘VP8X’ chunk (can currently contain "VP8 " or "VP8L") but as I do not interpret that VP8 data, why restrict the reader to it? Maybe a future libwebp supports something else than VP8 after the WebP Header ...

- Martin

On Sept. 1, 2012, 9:23 p.m., Martin Koller wrote:

Re: Review Request: add kimgio WebP image format plugin

By Christoph Feck at 09/01/2012 - 09:20

Nice work, much requested. See issues below.

kimgio/webp.cpp
<http://git.reviewboard.kde.org/r/106300/#comment14513>

We need to be more verbose here, it has to state the versions, and a pointer where to get the license text. Please check the KDE licensing page for correct texts.

kimgio/webp.cpp
<http://git.reviewboard.kde.org/r/106300/#comment14514>

Needs to check image.isValid() before writing data via scanLine() pointers. The allocation may fail because of too large width*height.

Additionally, I would like the loader to return a Format_RGB32 image, when the data did not contain an alpha component.

kimgio/webp.cpp
<http://git.reviewboard.kde.org/r/106300/#comment14515>

Same here, should handle pure RGB case.

kimgio/webp.cpp
<http://git.reviewboard.kde.org/r/106300/#comment14516>

"header.size() == 12 && ..."

Otherwise, it would return true for the file "RIFFWEBP" :)

- Christoph Feck

On Sept. 1, 2012, 8:33 a.m., Martin Koller wrote:

Re: Review Request: add kimgio WebP image format plugin

By Christoph Feck at 09/01/2012 - 20:07

Sorry, I meant isNull(). Since a constructor has no return code, a failed allocation results in a null image.

If your version has no way to detect alpha component, then use a newer version. If the newest libwebp does not return that information, then wait until its developers wake up (a joggle them).

- Christoph

On Sept. 1, 2012, 9:23 p.m., Martin Koller wrote:

Re: Review Request: add kimgio WebP image format plugin

By Martin Koller at 09/01/2012 - 17:12

Note: I copied the license lines from xview.cpp
I've changed my files now according to the KDE policy, but probably someone should also change the older files

QImage has no isValid() method. Also I find no hint in QImage docu that it _can_ be invalid after this constructor.

Regarding alpha: the version of the WebP library I have installed on openSuse 12.1 (0.1.2) does not have
a function to check if the image has alpha data.
On the Google page <a href="https://developers.google.com/speed/webp/docs/api?hl=ru" title="https://developers.google.com/speed/webp/docs/api?hl=ru">https://developers.google.com/speed/webp/docs/api?hl=ru</a>
I find the function WebPGetFeatures() but as said, my version does not have it.

Same problem as above.

- Martin

On Sept. 1, 2012, 8:33 a.m., Martin Koller wrote: