New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement gdk-pixbuf loader #107

Merged
merged 5 commits into from Feb 6, 2019

Conversation

Projects
None yet
3 participants
@ohwgiles
Copy link
Contributor

ohwgiles commented Jan 29, 2019

Providing a gdk-pixbuf loader module means applications such as gpicview and pcmanfm automatically gain HEIF support.

There are probably some improvements to be made, it would be especially nice to use the embedded thumbnails, but application code doesn't seem to reliably indicate to gdk-pixbuf that a thumbnail could be returned

@ohwgiles ohwgiles force-pushed the ohwgiles:feature/gdk-pixbuf branch from 25759f0 to d67258f Jan 29, 2019

@farindk

This comment has been minimized.

Copy link
Contributor

farindk commented Feb 1, 2019

Thank you, this is great.

@fancycode : I guess we will have to put this into a separate package in order to remove the dependency on gdk-pixbuf for the main library. Can you have a look how to do this?

Note: we might also need a post-install script that runs

cache_file=`pkg-config gdk-pixbuf-2.0 --variable gdk_pixbuf_cache_file`
gdk-pixbuf-query-loaders >$cache_file

to immediately update the cache of existing loaders.

@farindk

This comment has been minimized.

Copy link
Contributor

farindk commented Feb 1, 2019

I have pulled this into the feature branch ohwgiles-feature/gdk-pixbuf where we can add the necessary changes.

To do:

  • cmake: compilation should be optional
  • autoconf: add compilation
@ohwgiles

This comment has been minimized.

Copy link
Contributor Author

ohwgiles commented Feb 3, 2019

Thanks for the positive reception. I've added a commit to optionally compile the loader. I left it to default ON but maybe you want to change that.

I'm not sure I can help with the autoconf.

Regarding the post-install script to update the cache, I suggest this is better left to distributions, since as soon as you build deb/rpm/etc it has to be done differently, not to mention when cross-compiling.

@fancycode
Copy link
Member

fancycode left a comment

Thanks! I added support for autoconf/automake and integrated building with Travis CI in fcfed0a.

Travis: https://travis-ci.org/strukturag/libheif/builds/488412771
AppVeyor: https://ci.appveyor.com/project/strukturag/libheif/builds/22102433

Please cherry-pick this to your branch and also resolve the commented issues.

* This file is part of libheif.
*
* libheif is free software: you can redistribute it and/or modify
* it under the terms of the GNU Lesser General Public License as

This comment has been minimized.

@fancycode

fancycode Feb 4, 2019

Member

You mention GPL here (https://github.com/strukturag/libheif/pull/107/files#diff-6206122bfef85e4e34d08571ef26db03R169) but the file has a LGPL header, please either change the header to the GPL version or mention LGPL below. Also, please put a COPYING file in the gdk-pixbuf folder if it's GPL - similar to our examples which are MIT licensed or the GO code (which is GPL).

This comment has been minimized.

@ohwgiles

ohwgiles Feb 5, 2019

Author Contributor

My mistake - I intended LGPLv3. Fix pushed.

@@ -63,3 +63,8 @@ endif()

add_subdirectory (examples)
add_subdirectory (libheif)

option(WITH_GDK_PIXBUF_LOADER "Compile a gdk-pixbuf loader plugin" ON)

This comment has been minimized.

@fancycode

fancycode Feb 4, 2019

Member

This check is not needed. The subdirectory can determine if gdk-pixbuf is available and conditionally build it then.

This comment has been minimized.

@ohwgiles

ohwgiles Feb 5, 2019

Author Contributor

Well yes, but IMO it's nice to always be explicit about desired features and then use find_package(.. REQUIRED). Anyway this is also fine.


add_library(pixbufloader-heif MODULE pixbufloader-heif.c)

target_compile_definitions(pixbufloader-heif PRIVATE "-DGDK_PIXBUF_ENABLE_BACKEND")

This comment has been minimized.

@fancycode

fancycode Feb 4, 2019

Member

I moved the GDK_PIXBUF_ENABLE_BACKEND define in the code to avoid duplication with the autoconf/automake scripts.

This comment has been minimized.

@ohwgiles

ohwgiles Feb 5, 2019

Author Contributor

👍

@fancycode

This comment has been minimized.

Copy link
Member

fancycode commented Feb 4, 2019

Regarding the post-install script to update the cache, I suggest this is better left to distributions, since as soon as you build deb/rpm/etc it has to be done differently, not to mention when cross-compiling.

I agree, this is something that should be done by the packaging.

@fancycode

This comment has been minimized.

Copy link
Member

fancycode commented Feb 4, 2019

@fancycode : I guess we will have to put this into a separate package in order to remove the dependency on gdk-pixbuf for the main library. Can you have a look how to do this?

Similar as we do with the thumbnailer: the package libgdk-pixbuf2.0-dev will be added as build-dependency and the resulting library will be put in a separate binary package that then has a dependency on the libheif package, libgdk-pixbuf2.0-0 and whatever else will be pulled in during linking. This (new) binary package will also run the post-install script to update the cache.

@farindk

This comment has been minimized.

Copy link
Contributor

farindk commented Feb 4, 2019

Yes, sounds good.

@ohwgiles ohwgiles force-pushed the ohwgiles:feature/gdk-pixbuf branch from c7eb5d6 to 07c9a6e Feb 5, 2019

remove cmake option() for gdk-pixbuf loader
using if(GDK2_PIXBUF_FOUND) instead

@fancycode fancycode merged commit 9d96532 into strukturag:master Feb 6, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fancycode

This comment has been minimized.

Copy link
Member

fancycode commented Feb 6, 2019

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment