Skip to content

webp export: lossless encoding is now truely lossless (bug #12061)#1866

Closed
edgomez wants to merge 1 commit into
darktable-org:masterfrom
edgomez:bugfix/webp-lossless-export
Closed

webp export: lossless encoding is now truely lossless (bug #12061)#1866
edgomez wants to merge 1 commit into
darktable-org:masterfrom
edgomez:bugfix/webp-lossless-export

Conversation

@edgomez
Copy link
Copy Markdown
Member

@edgomez edgomez commented Dec 1, 2018

  • do not go through RGB->YUV420 conversion when lossless is set
  • be a bit paranoid and set alpha encoding to "exact"
  • reorder the code a bit to make it clearer

Though the encoding is now lossless, the command used to fingerprint
the RGB data in https://redmine.darktable.org/issues/12061 still doesn't
match :-(

But if one goes through:
$ convert foo.webp webp.rgb
$ convert foo.tif tif.rgb
$ diff -b tif.rgb webp.rgb && echo they do match
they do match

So i don't understand what IM does differently in identify and convert
that are both part of IM programs.

@TurboGit TurboGit self-requested a review December 1, 2018 19:14
@TurboGit TurboGit self-assigned this Dec 1, 2018
@TurboGit TurboGit added the feature: enhancement current features to improve label Dec 1, 2018
@TurboGit TurboGit added this to the 2.8 milestone Dec 1, 2018
…org#12061)

- do not go through RGB->YUV420 conversion when lossless is set
- be a bit paranoid and set alpha encoding to "exact"
- reorder the code a bit to make it clearer

Though the encoding is now lossless, the command used to fingerprint
the RGB data in https://redmine.darktable.org/issues/12061 still doesn't
match :-(

But if one goes through:
$ convert foo.webp webp.rgb
$ convert foo.tif tif.rgb
$ diff -b tif.rgb webp.rgb && echo they do match
they do match

So i don't understand what IM does differently in identify and convert
that are both part of IM programs.
@edgomez edgomez force-pushed the bugfix/webp-lossless-export branch from 3d3953c to 1f49555 Compare December 2, 2018 23:03
@TurboGit
Copy link
Copy Markdown
Member

I did some test and found that the lossless mode is now probably 20x slower.

I took a RAW, exported lossless, 95%. It took 10s or so with previous version and more than 3 minutes with this version.

Could this be because of the alpha encoding to "exact"? Is that even necessary for pictures?

@edgomez
Copy link
Copy Markdown
Member Author

edgomez commented Dec 28, 2018

Could this be because of the alpha encoding to "exact"? Is that even necessary for pictures?

It probably comes from the fact that the old code used YUV encoding and was going through the usual video encoding pipeline of VP8 codec w/ very low quantization parameter, or quantization disabled altogether.

The new code uses a newer encoding mode that got designed when WEBP team realized they were missing true RGB lossless encoder.

And I guess that lossless encoder didn't receive much optmization love as the video one.

But it's just a wild guess, I didn't follow WEBP/VP8 encoders' dev much.

@TurboGit
Copy link
Copy Markdown
Member

Ok, thanks. Merged to master.

@TurboGit TurboGit closed this Dec 29, 2018
@LebedevRI
Copy link
Copy Markdown
Member

This is so fun, pr is closed, yet it is actually merged, and the commit has no mention of the PR :)
Anyways, this is broken:

[  238s] Scanning dependencies of target exr
[  238s] make[2]: Leaving directory '/home/abuild/rpmbuild/BUILD/darktable-2.7.0~git78.fe7a2f997/build'
[  238s] /home/abuild/rpmbuild/BUILD/darktable-2.7.0~git78.fe7a2f997/src/imageio/format/webp.c:149:9: error: 'WebPConfig {aka struct WebPConfig}' has no member named 'exact'
[  238s]    config.exact = !!(config.lossless);
[  238s]          ^

https://build.opensuse.org/public/build/graphics:darktable:master/openSUSE_Leap_42.3/x86_64/darktable/_log

@TurboGit
Copy link
Copy Markdown
Member

Roman, nothing fun to me. This has been rebase and merged manually to allow merging into the darktable-2.6.x release branch if needed. And in this case GitHub do not detect that the PR is merged.

Also, it is not broken. I do test every commit :) It is broken in one platform and probably because the webp library is too old here. I'll comment out the exact field for now.

@LebedevRI
Copy link
Copy Markdown
Member

LebedevRI commented Dec 29, 2018

It is broken in one platform and probably because the webp library is too old here. I'll comment out the exact field for now.

Looks like it, exact first appeared in https://github.com/webmproject/libwebp/blob/v0.5.0/src/webp/encode.h#L139-L142, it was not in https://github.com/webmproject/libwebp/blob/v0.4.4/src/webp/encode.h
(i would guess either the libwebp version check in cmake should be bumped, or the field should only be used if libwebp is v0.5.0 or newer)

@TurboGit
Copy link
Copy Markdown
Member

Right, just check this. So I'll remove this field. I'm currently testing this works ok with recent webp.

@edgomez edgomez deleted the bugfix/webp-lossless-export branch March 17, 2019 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: enhancement current features to improve

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants