After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 630748 - display filters do not work
display filters do not work
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: User Interface
git master
Other All
: Normal normal
: 2.8
Assigned To: GIMP Bugs
GIMP Bugs
: 636756 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-09-27 17:34 UTC by Serge Gavrilov
Modified: 2011-01-25 19:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Preliminary (not-submit-ready) patch to make display filters work again (24.78 KB, patch)
2011-01-23 07:36 UTC, Omari Stephens
needs-work Details | Review
Restores the stock API and renames the new one with a _surface suffix (44.11 KB, patch)
2011-01-23 18:13 UTC, Omari Stephens
none Details | Review
Fix a missing-parens-in-macro bug for the ARGB32 getters and setters (3.61 KB, patch)
2011-01-24 19:54 UTC, Omari Stephens
committed Details | Review
Updated patch which fixes some things (44.63 KB, patch)
2011-01-24 20:05 UTC, Omari Stephens
none Details | Review
Merge candidate 1: I think all the issues are ironed out (46.33 KB, patch)
2011-01-24 22:42 UTC, Omari Stephens
committed Details | Review

Description Serge Gavrilov 2010-09-27 17:34:05 UTC
In the current git GIMP display filters are currently disabled because of the port to cairo

Thanks!
Comment 1 Omari Stephens 2011-01-22 23:55:31 UTC
*** Bug 636756 has been marked as a duplicate of this bug. ***
Comment 2 Omari Stephens 2011-01-23 07:36:19 UTC
Created attachment 179069 [details] [review]
Preliminary (not-submit-ready) patch to make display filters work again

Would be nice if someone could review this.  Note that it doesn't build with pygimp enabled right now; will post an updated patch when I figure that bit out
Comment 3 Michael Natterer 2011-01-23 14:59:18 UTC
Very nice :)

Some comments:

- we can't change public functions in the 2.x series, so please deprecate
  the existing API and add new one, e.g. convert_surface().
- same for the module API: use one of the reserved slots for the
  new vtable entry.
- the lcms calls don't work as expected because the pixel format
  in the cairo surface is ARGB *premultiplied*, and the
  GIMP_CAIRO_ARGB32_GET/SET_PIXEL() macros take care of that,
  however I hope that lcms understands premultiplied ARGB directly.
- coding style wise, not much to complain, just that we have
  a space before the ( in function calls.
Comment 4 Omari Stephens 2011-01-23 18:13:16 UTC
Created attachment 179089 [details] [review]
Restores the stock API and renames the new one with a _surface suffix

- Compiles with python enabled, but I probably need to do something to force the API to be exposed by pygimp.
- Need to do #ifndef GIMP_DISABLE_DEPRECATED in a bunch of places still
- Need to fix lcms stuff still
- Need to add spaces before open-parens :o)
Comment 5 Omari Stephens 2011-01-24 19:54:50 UTC
Created attachment 179215 [details] [review]
Fix a missing-parens-in-macro bug for the ARGB32 getters and setters

The new patch depends on this one to compile successfully
Comment 6 Omari Stephens 2011-01-24 20:05:36 UTC
Created attachment 179217 [details] [review]
Updated patch which fixes some things

This patch is getting pretty close to done.  If someone has a big-endian machine they could test on (especially the lcms and colorblind stuff), I would appreciate it; I don't have access to any.

Stuff fixed in this version:
 - Undo pre-mul before invoking lcms, and redo pre-mul afterward.  Context:
   http://www.mail-archive.com/lcms-user@lists.sourceforge.net/msg02767.html
 - The new vtable entry in _GimpColorDisplayClass was in the wrong place, which
   would break ABI compatibility.  Moved it to the right spot (I believe).
 - Atonement for my whitespace transgressions against the world

Still left to do:
 - Update pygimp to export the _surface APIs
 - Update API documentation
Comment 7 Omari Stephens 2011-01-24 22:42:24 UTC
Created attachment 179244 [details] [review]
Merge candidate 1: I think all the issues are ironed out

I think this one is ready to go, pending people finding other issues.  Changes from the previous patch:
 - Calls cairo_surface_flush before and cairo_surface_mark_dirty after each display filter
 - Adds inline comments about the non-"_surface" convert APIs being deprecated
 - Adds the _surface APIs in pygimp, and marks the non-"_surface" APIs as deprecated
Comment 8 Michael Natterer 2011-01-25 19:29:50 UTC
Pushed a slightly modified version of the last patch:

commit 5cae0bf65c9720c3b7b142240724c42e18bbca14
Author: Omari Stephens <xsdg@xsdg.org>
Date:   Sun Jan 23 07:28:33 2011 +0000

    Bug 630748 - display filters do not work
    
    Create and use Cairo-compatible API for display filters. Also
    includes logic changes to the display filters to deal with cairo's
    ARGB32 pre-multiplied buffer format.

 app/display/gimpdisplayshell-render.c  |   31 +++-----
 libgimpwidgets/gimpcolordisplay.c      |   43 +++++++++++
 libgimpwidgets/gimpcolordisplay.h      |    9 ++-
 libgimpwidgets/gimpcolordisplaystack.c |   42 ++++++++++
 libgimpwidgets/gimpcolordisplaystack.h |   38 +++++-----
 modules/display-filter-color-blind.c   |  131 ++++++++++++++++----------------
 modules/display-filter-gamma.c         |   85 +++++++++++----------
 modules/display-filter-high-contrast.c |   83 +++++++++++----------
 modules/display-filter-lcms.c          |  119 ++++++++++++++++++-----------
 modules/display-filter-proof.c         |  112 +++++++++++++++++-----------
 plug-ins/pygimp/gimpui.defs            |   20 +++++
 11 files changed, 444 insertions(+), 269 deletions(-)