Details

Reviewers

Sergey Sharybin (sergey)
Sybren A. Stüvel (sybren)
Brecht Van Lommel (brecht)
Ray Molenkamp (LazyDodo)

Commits

rB4fd0a69d7ba8: ImBuf: Add support for WebP image format

Summary

Currently only support single image frames, no animation possible.

If quality slider is set to 100 then lossless compression will be used,
otherwise lossy compression is used.

Gives about 35% of file save when re-saving splash screens with lossless
compression.
Also saves much faster, up to 15x faster than PNG with a better compression ratio as a plus.

Known Issues:

macOS support, in theory should work unsure about mac on arm
BW color mode support -- WebP does not have a specific grayscale encoder. We could write the image as a grayscale 3 byte image however that goes beyond what the color_mode toggle is designed for.

Libraries for testing

Google provides recompiled libraries which can be used for drop in testing. I used version 1.2.2 (latest)

https://storage.googleapis.com/downloads.webmproject.org/releases/webp/index.html

Diff Detail

Repository

rB Blender

Branch

arcpatch-D1598 (branched from master)

Build Status

Buildable 21220
Build 21220: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Still some minor cleanup and fixes needed

Aaron Carlisle (Blendify) marked 3 inline comments as done.Mar 12 2022, 7:35 AM

Cleanup: formating, change from int to bool, fix merge errors, fix typo in retrun type

Harbormaster completed remote builds in B21001: Diff 49204.Mar 13 2022, 12:13 AM

Cleanup: Use SPDX license header
Updates to findwebp

Harbormaster completed remote builds in B21003: Diff 49206.Mar 13 2022, 1:15 AM

Aaron Carlisle (Blendify) marked an inline comment as done.Mar 13 2022, 1:17 AM

Fix findwebp

Harbormaster completed remote builds in B21004: Diff 49207.Mar 13 2022, 1:50 AM

Fix: Incorrect struct init

Harbormaster completed remote builds in B21005: Diff 49208.Mar 13 2022, 2:19 AM

Aaron Carlisle (Blendify) edited the summary of this revision. (Show Details)Mar 13 2022, 2:22 AM

Fix linking error
Fix building without webp

Harbormaster completed remote builds in B21006: Diff 49209.Mar 13 2022, 6:10 AM

Regarding BW image support, WebP does not have a special grayscale color space.

The color mode option is primarily used to set the image format to this more optimized color space.
If artist want to convert an image to black and white, they should use the compositor for that.

If we want this to work then we should add support for all image formats in which case we would have to
convert the image to grayscale then convert the 1 byte image back to 3 bytes for image formats lacking a grayscale encoding.

With that being said this patch is read for review, the one todo is macOS support, I leave that up to Platform: macOS

Aaron Carlisle (Blendify) edited the summary of this revision. (Show Details)Mar 13 2022, 6:36 AM

Aaron Carlisle (Blendify) edited the summary of this revision. (Show Details)Mar 13 2022, 6:38 AM

Initial macOS support

Harbormaster completed remote builds in B21009: Diff 49220.Mar 13 2022, 9:22 PM

Brecht Van Lommel (brecht) requested changes to this revision.Mar 13 2022, 10:34 PM

Brecht Van Lommel (brecht) added a subscriber: Brecht Van Lommel (brecht).

Brecht Van Lommel (brecht) added inline comments.

build_files/cmake/Modules/FindWebP.cmake
24–28	Leave out all these additional paths except `/opt/lib/webp`, for consistency with other find modules.
build_files/cmake/platform/platform_win32.cmake
346	Capitalize correctly to avoid cmake warning: `windows_find_package(WebP)`
source/blender/imbuf/CMakeLists.txt
53	Only add this to the list of sources if webp is enabled.
source/blender/imbuf/intern/util.c
44–67	Not sure where these additional spaces come from.
source/blender/imbuf/intern/webp.c
50	Useless #ifdef, the entire file won't build if this is not enabled.
55	Use `fprintf(stderr,` for errors, here and a few other places.
88–89	Did you check that this saving without alpha works correctly? From a quick look at the webp code, it seems to assume a pixel stride of 3, but in Blender, byte images always have a pixel stride of 4. You might have to temporarily allocate a RGBRGBRGB buffer from the existing RGBARGBARGBA buffer to make this work. Or not support saving without alpha channel.
100–101	Same.

This revision now requires changes to proceed.Mar 13 2022, 10:34 PM

Brecht Van Lommel (brecht) removed a reviewer: Ray Molenkamp (LazyDodo). Brecht Van Lommel (brecht) removed 2 blocking reviewer(s): Sergey Sharybin (sergey), Sybren A. Stüvel (sybren).Mar 13 2022, 10:35 PM

I guess we could leave out the color mode and options for saving as BW / RGB / RGBA for this file format, adding a BKE_imtype_supports_color_mode and hiding the color_mode in uiTemplateImageSettings if true.

CMakeLists.txt
287	This must be `OFF` if this gets committed before the libraries.
source/blender/imbuf/intern/webp.c
47	If we support RGB saving, loading should also preserve the information and 24 if it's an RGB image without alpha. Note that `ibuf->rect` is not affected by this and the following code in this function can remain the same.

Supporting writing of BW and RGB channels is not very complicated though, just a matter of allocating an RGBRGBRGB buffer and encoding a grayscale or RGB image into it, so might as well I guess.

Address simple review comments

Harbormaster completed remote builds in B21010: Diff 49223.Mar 13 2022, 11:21 PM

In D1598#383036, @Brecht Van Lommel (brecht) wrote:

I guess we could leave out the color mode and options for saving as BW / RGB / RGBA for this file format, adding a BKE_imtype_supports_color_mode and hiding the color_mode in uiTemplateImageSettings if true.

So the enum should be dynamic based on BKE_imtype_supports_color_mode already but there seems to be a bug and this only works for the file output dialog.

In D1598#383039, @Brecht Van Lommel (brecht) wrote:

Supporting writing of BW and RGB channels is not very complicated though, just a matter of allocating an RGBRGBRGB buffer and encoding a grayscale or RGB image into it, so might as well I guess.

We do not do this for other formats, take EXR for example.

source/blender/imbuf/intern/util.c
44–67	Not sure either, clang format is insisting on it though.
source/blender/imbuf/intern/webp.c
47	So I have this: just not sure how to get the colorspace mode from the webp file: const WebPEncCSP csp_mode = if (WebPIsAlphaMode(csp_mode)) { ibuf = IMB_allocImBuf(width, height, 32, 0); } else { ibuf = IMB_allocImBuf(width, height, 24, 0); }
88–89	Yes alpha seems to work correctly from my test.

Use fprintf

Harbormaster completed remote builds in B21011: Diff 49226.Mar 14 2022, 12:06 AM

Merge branch 'master' into arcpatch-D1598

Harbormaster completed remote builds in B21012: Diff 49227.Mar 14 2022, 12:06 AM

Aaron Carlisle (Blendify) marked 2 inline comments as not done.Mar 14 2022, 12:11 AM

Aaron Carlisle (Blendify) added inline comments.

source/blender/imbuf/intern/webp.c
88–89	Err I see what you mean, saving an RGBA image as RGB doesnt currently work

In D1598#383064, @Aaron Carlisle (Blendify) wrote:

In D1598#383036, @Brecht Van Lommel (brecht) wrote:

I guess we could leave out the color mode and options for saving as BW / RGB / RGBA for this file format, adding a BKE_imtype_supports_color_mode and hiding the color_mode in uiTemplateImageSettings if true.

So the enum should be dynamic based on BKE_imtype_supports_color_mode already but there seems to be a bug and this only works for the file output dialog.

In D1598#383039, @Brecht Van Lommel (brecht) wrote:

Supporting writing of BW and RGB channels is not very complicated though, just a matter of allocating an RGBRGBRGB buffer and encoding a grayscale or RGB image into it, so might as well I guess.

We do not do this for other formats, take EXR for example.

Ah, I see the enum is already dynamic based on BKE_imtype_valid_channels. Just supporting RGB and RGBA then seems best, since that's what the file format supports.

Saving RGB should be simple with a bit of code like this:

const size_t num_pixels = ibuf->x * ibuf->y;
const uint8_t *rgba_rect = (uint8_t *)ibuf->rect;
uint8_t *rgb_rect = MEM_mallocN(sizeof(uint8_t) * num_pixels * 3, "webp rgb_rect");
for (int i = 0; i < num_pixels; i++) {
  rgb_rect[i * 3 + 0] = rgba_rect[i * 4 + 0];
  rgb_rect[i * 3 + 1] = rgba_rect[i * 4 + 1];
  rgb_rect[i * 3 + 2] = rgba_rect[i * 4 + 2];
}

/* use rgb_rect for output .. */

MEM_freeN(rgb_rect);

source/blender/imbuf/intern/webp.c
63	All error printfs in this file should use `fprintf(stderr`.
63	Ignore this, was looking at an older diff.

Brecht Van Lommel (brecht) requested changes to this revision.Mar 14 2022, 3:09 AM

This revision now requires changes to proceed.Mar 14 2022, 3:09 AM

build some libs for this, couple of issues

FindWebP.cmake doesn't work at all with static libs, this seems to do the trick for me, but not my platform, so it's not been super extensively tested.

# SPDX-License-Identifier: BSD-3-Clause
# Copyright 2022 Blender Foundation.

# - Find WebP library
# Find the native WebP includes and library
# This module defines
#  WEBP_INCLUDE_DIRS, where to find WebP headers, Set when WebP is found.
#  WEBP_LIBRARIES, libraries to link against to use WebP.
#  WEBP_ROOT_DIR, The base directory to search for WebP.
#                 This can also be an environment variable.
#  WEBP_FOUND, If false, do not try to use WebP.
#
# also defined, but not for general use are
#  WEBP_LIBRARY, where to find the WEBP library.

# If WEBP_ROOT_DIR was defined in the environment, use it.
IF(NOT WEBP_ROOT_DIR AND NOT $ENV{WEBP_ROOT_DIR} STREQUAL "")
  SET(WEBP_ROOT_DIR $ENV{WEBP_ROOT_DIR})
ENDIF()

SET(_webp_SEARCH_DIRS
  ${WEBP_ROOT_DIR}
  /opt/lib/webp
)

FIND_PATH(WEBP_INCLUDE_DIR
  NAMES
    webp/types.h
  HINTS
    ${_webp_SEARCH_DIRS}
  PATH_SUFFIXES
    include
)

SET(_webp_FIND_COMPONENTS
    webp
    webpdecoder
    webpmux
    webpdemux
  )

SET(_webp_LIBRARIES)
FOREACH(COMPONENT ${_webp_FIND_COMPONENTS})
  STRING(TOUPPER ${COMPONENT} UPPERCOMPONENT)

  FIND_LIBRARY(WEBP_${UPPERCOMPONENT}_LIBRARY
    NAMES
      ${COMPONENT}
    NAMES_PER_DIR
    HINTS
      ${_webp_SEARCH_DIRS}
    PATH_SUFFIXES
      lib64 lib lib/static
    )
  LIST(APPEND _webp_LIBRARIES "${WEBP_${UPPERCOMPONENT}_LIBRARY}")
ENDFOREACH()

IF(${WEBP_WEBP_LIBRARY_NOTFOUND})
  set(WEBP_FOUND FALSE)
ELSE()
  # handle the QUIETLY and REQUIRED arguments and set WEBP_FOUND to TRUE if
  # all listed variables are TRUE
  INCLUDE(FindPackageHandleStandardArgs)
  FIND_PACKAGE_HANDLE_STANDARD_ARGS(WebP DEFAULT_MSG _webp_LIBRARIES WEBP_INCLUDE_DIR)

  IF(WEBP_FOUND)
    get_filename_component(WEBP_LIBRARY_DIR ${WEBP_WEBP_LIBRARY} DIRECTORY)
    SET(WEBP_INCLUDE_DIRS ${WEBP_INCLUDE_DIR})
    SET(WEBP_LIBRARIES ${_webp_LIBRARIES})
  ELSE()
    SET(WEBPL_PUGIXML_FOUND FALSE)
  ENDIF()
ENDIF()

MARK_AS_ADVANCED(
  WEBP_INCLUDE_DIR
  WEBP_LIBRARY_DIR
)

the cycles_target_link_libraries macro inside /intern/cycles/cmake/macros.cmake needs the following change for cycles_test to link

diff --git a/intern/cycles/cmake/macros.cmake b/intern/cycles/cmake/macros.cmake
index 0f2e1b50434..e69e31f8e52 100644
--- a/intern/cycles/cmake/macros.cmake
+++ b/intern/cycles/cmake/macros.cmake
@@ -101,6 +101,7 @@ macro(cycles_target_link_libraries target)
     ${PNG_LIBRARIES}
     ${JPEG_LIBRARIES}
     ${TIFF_LIBRARY}
+    ${WEBP_LIBRARIES}
     ${OPENJPEG_LIBRARIES}
     ${OPENEXR_LIBRARIES}
     ${OPENEXR_LIBRARIES} # For circular dependencies between libs.

Regardless if this diff lands, the cmake changes WILL have to land as oiio now requires linkage to webp, i've landed the required cmake changes to properly link, and the two above changes to the tmp_lib_update_32 so the platform devs can at least test if blender+tests link correctly

3a) while windows builds on the 3.2 update branch, linux seems to have a link ordering issue I couldn't easily track down, will probably also plague mac...

Update cmake scripts with changes from Ray
Merge branch 'master' into arcpatch-D1598

Harbormaster completed remote builds in B21214: Diff 49623.Mar 23 2022, 9:43 PM

Aaron Carlisle (Blendify) marked 3 inline comments as done.Mar 23 2022, 9:45 PM

Merge branch 'master' into arcpatch-D1598
Fix reading and writing RGB buffer

Harbormaster completed remote builds in B21220: Diff 49630.Mar 24 2022, 2:32 AM

Aaron Carlisle (Blendify) marked 4 inline comments as done.Mar 24 2022, 2:33 AM

Brecht Van Lommel (brecht) requested changes to this revision.Mar 24 2022, 2:57 AM

Brecht Van Lommel (brecht) added inline comments.

source/blender/imbuf/intern/webp.c

Handle errors:

if (WebPGetFeatures(mem, size, &features) != VP8_STATUS_OK) {
  fprintf(stderr, "WebP: Failed to parse features\n");
  return NULL;
}

Deduplicate this code by restructuring:

if (bytesperpixel == 3) {
  .. 
  if (ibuf->foptions.quality == 100.0f) {
  }
  else {
  }
  ..
}
else if (bytesperpixel == 4) {
  if (ibuf->foptions.quality == 100.0f) {
  }
  else {
  }
}

This revision now requires changes to proceed.Mar 24 2022, 2:57 AM

Cleanup: Deduplicate and simplify code

There are some issue related to cmake still
I am discussing the following fix with Ray:

https://developer.blender.org/P2850

Harbormaster completed remote builds in B21221: Diff 49637.Mar 24 2022, 6:49 AM

remove the webpdecoder lib from FindWebP

Harbormaster completed remote builds in B21238: Diff 49677.Mar 24 2022, 9:52 PM

The code looks correct to me now.

Given you make that small fix before landing, this seems ok

build_files/cmake/platform/platform_win32.cmake
350	`WEBP_LIBRARIES` and needs the other two libs

This revision is now accepted and ready to land.Mar 24 2022, 10:12 PM

Closed by commit rB4fd0a69d7ba8: ImBuf: Add support for WebP image format (authored by Aaron Carlisle (Blendify)). · Explain WhyMar 24 2022, 11:25 PM

This revision was automatically updated to reflect the committed changes.

Aaron Carlisle (Blendify) added a commit: rB4fd0a69d7ba8: ImBuf: Add support for WebP image format.

Tolkfan (Tolkfan) awarded a token.Apr 19 2022, 8:03 PM

Paul Kotelevets (1D_Inc) awarded a token.Apr 20 2022, 10:11 PM

ImBuf: Add support for WebP image format
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 49630

CMakeLists.txt

build_files/cmake/Modules/FindWebP.cmake

build_files/cmake/platform/platform_apple.cmake

build_files/cmake/platform/platform_unix.cmake

build_files/cmake/platform/platform_win32.cmake

intern/cycles/cmake/macros.cmake

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/image_format.cc

source/blender/editors/space_file/CMakeLists.txt

source/blender/editors/space_image/CMakeLists.txt

source/blender/imbuf/CMakeLists.txt

source/blender/imbuf/IMB_imbuf_types.h

source/blender/imbuf/intern/IMB_filetype.h

source/blender/imbuf/intern/filetype.c

source/blender/imbuf/intern/util.c

source/blender/imbuf/intern/webp.c

source/blender/makesdna/DNA_scene_types.h

source/blender/makesrna/intern/CMakeLists.txt

source/blender/makesrna/intern/rna_scene.c

source/blender/python/intern/CMakeLists.txt

ImBuf: Add support for WebP image formatClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 49630

CMakeLists.txt

build_files/cmake/Modules/FindWebP.cmake

build_files/cmake/platform/platform_apple.cmake

build_files/cmake/platform/platform_unix.cmake

build_files/cmake/platform/platform_win32.cmake

intern/cycles/cmake/macros.cmake

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/image_format.cc

source/blender/editors/space_file/CMakeLists.txt

source/blender/editors/space_image/CMakeLists.txt

source/blender/imbuf/CMakeLists.txt

source/blender/imbuf/IMB_imbuf_types.h

source/blender/imbuf/intern/IMB_filetype.h

source/blender/imbuf/intern/filetype.c

source/blender/imbuf/intern/util.c

source/blender/imbuf/intern/webp.c

source/blender/makesdna/DNA_scene_types.h

source/blender/makesrna/intern/CMakeLists.txt

source/blender/makesrna/intern/rna_scene.c

source/blender/python/intern/CMakeLists.txt

ImBuf: Add support for WebP image format
ClosedPublic
Actions

Revision Contents
Changeset List