Re: [patches] Add a color management framework to weston
I'd suggest that client should use subsurfaces if they want multiple colorspaces in a window. They might have to do that anyway since they may want a different pixel format. On Thu, May 2, 2013 at 8:58 AM, Pekka Paalanen wrote: > On Wed, 1 May 2013 17:03:30 -0400 > Kristian Høgsberg wrote: > > > On Mon, Apr 22, 2013 at 01:30:28PM +0300, Pekka Paalanen wrote: > > > On Fri, 5 Apr 2013 14:23:50 -0500 > > > Thomas Daede wrote: > > > > > > > I am not sure that doing the color conversion in the compositor > > > > is the correct place. Some of it has to be there to support vcgt, > > > > but for more general conversions, it gets complicated quickly. > > > > > > > > Color correction is most important for artists doing work in > > > > something like the GIMP. Programs like this (as of GIMP 3.0, at > > > > least) generally work in higher bit depths - 16 bit, half float, > > > > or sometimes 32 bit float. It's much better to do conversion in > > > > the higher bit depth, then dither to the final display depth. > > > > Doing this with the compositor involves supporting all of these > > > > texture formats for subsurfaces - also, the toolkit has to > > > > support subsurfaces, because generally the UI will be some lower > > > > bit depth, or not need correction. > > > > > > > > Converting bit depths in the compositor would also require an > > > > extra buffer allocation and copy in the compositor. > > > > > > > > RGB images are also not the only thing that needs to be color > > > > corrected - for example, 10bit YUV video streams might want to be > > > > color corrected too. If we were to go as far as converting bit > > > > depths in the compositor, it wouldn't be much to add this. > > > > > > > > I think that providing color space regions to the client, > > > > relative to the client's buffer, including vcgt settings, would > > > > shove a lot of complexity away to clients that are already used > > > > to dealing with it. > > > > > > I'm not sure we can do that. The regions can be arbitrarily complex > > > and non-linear shapes. > > > > It's not too different from how we handle opaque regions. We split up > > the surfaces into opaque and transparent parts and render them using > > different shaders already. > > Sorry, I read "providing color space regions to the client" as "the > server determines the areas of each output overlapping with the > surface", and communicated that to the clients. > > If the regions are communicated from clients to the server direction > only, then nevermind my comment. > > > Thanks, > pq > ___ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On Wed, 1 May 2013 17:03:30 -0400 Kristian Høgsberg wrote: > On Mon, Apr 22, 2013 at 01:30:28PM +0300, Pekka Paalanen wrote: > > On Fri, 5 Apr 2013 14:23:50 -0500 > > Thomas Daede wrote: > > > > > I am not sure that doing the color conversion in the compositor > > > is the correct place. Some of it has to be there to support vcgt, > > > but for more general conversions, it gets complicated quickly. > > > > > > Color correction is most important for artists doing work in > > > something like the GIMP. Programs like this (as of GIMP 3.0, at > > > least) generally work in higher bit depths - 16 bit, half float, > > > or sometimes 32 bit float. It's much better to do conversion in > > > the higher bit depth, then dither to the final display depth. > > > Doing this with the compositor involves supporting all of these > > > texture formats for subsurfaces - also, the toolkit has to > > > support subsurfaces, because generally the UI will be some lower > > > bit depth, or not need correction. > > > > > > Converting bit depths in the compositor would also require an > > > extra buffer allocation and copy in the compositor. > > > > > > RGB images are also not the only thing that needs to be color > > > corrected - for example, 10bit YUV video streams might want to be > > > color corrected too. If we were to go as far as converting bit > > > depths in the compositor, it wouldn't be much to add this. > > > > > > I think that providing color space regions to the client, > > > relative to the client's buffer, including vcgt settings, would > > > shove a lot of complexity away to clients that are already used > > > to dealing with it. > > > > I'm not sure we can do that. The regions can be arbitrarily complex > > and non-linear shapes. > > It's not too different from how we handle opaque regions. We split up > the surfaces into opaque and transparent parts and render them using > different shaders already. Sorry, I read "providing color space regions to the client" as "the server determines the areas of each output overlapping with the surface", and communicated that to the clients. If the regions are communicated from clients to the server direction only, then nevermind my comment. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On Mon, Apr 22, 2013 at 01:30:28PM +0300, Pekka Paalanen wrote: > On Fri, 5 Apr 2013 14:23:50 -0500 > Thomas Daede wrote: > > > I am not sure that doing the color conversion in the compositor is the > > correct place. Some of it has to be there to support vcgt, but for > > more general conversions, it gets complicated quickly. > > > > Color correction is most important for artists doing work in something > > like the GIMP. Programs like this (as of GIMP 3.0, at least) generally > > work in higher bit depths - 16 bit, half float, or sometimes 32 bit > > float. It's much better to do conversion in the higher bit depth, then > > dither to the final display depth. Doing this with the compositor > > involves supporting all of these texture formats for subsurfaces - > > also, the toolkit has to support subsurfaces, because generally the UI > > will be some lower bit depth, or not need correction. > > > > Converting bit depths in the compositor would also require an extra > > buffer allocation and copy in the compositor. > > > > RGB images are also not the only thing that needs to be color > > corrected - for example, 10bit YUV video streams might want to be > > color corrected too. If we were to go as far as converting bit depths > > in the compositor, it wouldn't be much to add this. > > > > I think that providing color space regions to the client, relative to > > the client's buffer, including vcgt settings, would shove a lot of > > complexity away to clients that are already used to dealing with it. > > I'm not sure we can do that. The regions can be arbitrarily complex and > non-linear shapes. It's not too different from how we handle opaque regions. We split up the surfaces into opaque and transparent parts and render them using different shaders already. Kristian ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On Fri, 5 Apr 2013 14:23:50 -0500 Thomas Daede wrote: > I am not sure that doing the color conversion in the compositor is the > correct place. Some of it has to be there to support vcgt, but for > more general conversions, it gets complicated quickly. > > Color correction is most important for artists doing work in something > like the GIMP. Programs like this (as of GIMP 3.0, at least) generally > work in higher bit depths - 16 bit, half float, or sometimes 32 bit > float. It's much better to do conversion in the higher bit depth, then > dither to the final display depth. Doing this with the compositor > involves supporting all of these texture formats for subsurfaces - > also, the toolkit has to support subsurfaces, because generally the UI > will be some lower bit depth, or not need correction. > > Converting bit depths in the compositor would also require an extra > buffer allocation and copy in the compositor. > > RGB images are also not the only thing that needs to be color > corrected - for example, 10bit YUV video streams might want to be > color corrected too. If we were to go as far as converting bit depths > in the compositor, it wouldn't be much to add this. > > I think that providing color space regions to the client, relative to > the client's buffer, including vcgt settings, would shove a lot of > complexity away to clients that are already used to dealing with it. I'm not sure we can do that. The regions can be arbitrarily complex and non-linear shapes. I also do not believe that solving the color correctness problem for a single surface spanning more than one monitor is worth all the pain and special cases, unless the color correction is done in the compositor. > > The compositor should be able to draw N polygons for the N > > intersections of a surface with each output, each using a different > > shader? You absolutely do not want to put an if based on a pixel > > value into a glsl shader. It is really slow. > > This is the correct way, I think. Splitting surfaces by monitor also > has other unrelated benefits, like being able to sync separately to > each monitor - it probably should go somewhere else. Weston already does it like that. Each output is a framebuffer, the desktop as a whole is not. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
Bill Spitzak wrote:
Maybe the client should be able to query the color space of the output
and can set the color space of it's buffer to the same one, this would
indicate that no color correction is needed. The compositor could still
color-correct it from one output's color space to another's if it is on
multiple outputs.
This is not as simple to set and as safe as an explicit opt out.
(Apple has had a lot of trouble as a result of doing their
color management opt out this way). It's hard to confirm that you've got
the same profile ("same" in what sense ? Same instance of the profile ?
Same bits ? Same notional colorspace ?)
You would need a special case anyway to implement a null transform,
because things like cLUT profiles have A2B and B2A tables that are not
perfect inverses.
You'd also want to be sure that a null transform is bit perfect, and
that something isn't lost in shaders etc.
If this idea is still attractve, I would suggest supporting device links,
making it reasonably simple to set a null device link.
Ideally a client should be able to do this with just some strings (ie
query the color space and send the same string as the buffer's color
space) so it does not have to open a CMS library that it might otherwise
not be using.
If that's the case, I'd suggest having a string that turns the color management
off. Much simpler and more direct as to the intention.
Graeme Gill.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
Thomas Daede wrote: I am not sure that doing the color conversion in the compositor is the correct place. Some of it has to be there to support vcgt, but for more general conversions, it gets complicated quickly. Hi, The expectation is that vcgt is per output (ie., it's loaded into the display cards RAMDAC), or loaded into the display itself using DDC. The display device color profile assumes this. So vcgt shouldn't need shaders. Color correction is most important for artists doing work in something like the GIMP. Programs like this (as of GIMP 3.0, at least) generally As well as this, there is a desire to color manage all GUI elements on a wide gamut display, to avoid a rather garish and hard to read result. I think that providing color space regions to the client, relative to the client's buffer, including vcgt settings, would shove a lot of complexity away to clients that are already used to dealing with it. One of the things driving the idea of tagging a colorspace and letting the display system handle color management by default, is that many/most applications don't deal with it. So I think that the idea of shoving off the learning curve about color to a larger group of applcation writers is not likely to be successful. This is the correct way, I think. Splitting surfaces by monitor also has other unrelated benefits, like being able to sync separately to each monitor - it probably should go somewhere else. Application writers particularly do not like dealing with the messiness of color managing their graphics elements when they happen to fall across more than one output. A number of applications get color management right on a single display, and fail badly with more than one display. Graeme Gill. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
Richard Hughes wrote: For GIMP I was going to just allow the app to opt-out a sub-surface from any kind of color management. i.e. do it all in the app. Maybe the client should be able to query the color space of the output and can set the color space of it's buffer to the same one, this would indicate that no color correction is needed. The compositor could still color-correct it from one output's color space to another's if it is on multiple outputs. Ideally a client should be able to do this with just some strings (ie query the color space and send the same string as the buffer's color space) so it does not have to open a CMS library that it might otherwise not be using. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On 5 April 2013 20:23, Thomas Daede wrote: > Color correction is most important for artists doing work in something > like the GIMP. Programs like this (as of GIMP 3.0, at least) generally > work in higher bit depths For GIMP I was going to just allow the app to opt-out a sub-surface from any kind of color management. i.e. do it all in the app. >> The compositor should be able to draw N polygons for the N intersections of >> a surface with each output, each using a different shader? >> You absolutely do not want to put an if based on a pixel value into a glsl >> shader. It is really slow. > > This is the correct way, I think. Splitting surfaces by monitor also > has other unrelated benefits, like being able to sync separately to > each monitor - it probably should go somewhere else. Right, I'll look into per-output shader code next week. Richard ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
I am not sure that doing the color conversion in the compositor is the correct place. Some of it has to be there to support vcgt, but for more general conversions, it gets complicated quickly. Color correction is most important for artists doing work in something like the GIMP. Programs like this (as of GIMP 3.0, at least) generally work in higher bit depths - 16 bit, half float, or sometimes 32 bit float. It's much better to do conversion in the higher bit depth, then dither to the final display depth. Doing this with the compositor involves supporting all of these texture formats for subsurfaces - also, the toolkit has to support subsurfaces, because generally the UI will be some lower bit depth, or not need correction. Converting bit depths in the compositor would also require an extra buffer allocation and copy in the compositor. RGB images are also not the only thing that needs to be color corrected - for example, 10bit YUV video streams might want to be color corrected too. If we were to go as far as converting bit depths in the compositor, it wouldn't be much to add this. I think that providing color space regions to the client, relative to the client's buffer, including vcgt settings, would shove a lot of complexity away to clients that are already used to dealing with it. > The compositor should be able to draw N polygons for the N intersections of a > surface with each output, each using a different shader? > You absolutely do not want to put an if based on a pixel value into a glsl > shader. It is really slow. This is the correct way, I think. Splitting surfaces by monitor also has other unrelated benefits, like being able to sync separately to each monitor - it probably should go somewhere else. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
Richard Hughes wrote: I've got to move onto other stuff next week, but I'd be really interested if anyone has inputs on how the sub-surface gamut mapping using shaders is going to work. The main complication seems to be that it's per-output, rather than per-surface. I've played with 1bit masks in the past, but this made the shader quite complicated. Ideas very welcome. Thanks. The compositor should be able to draw N polygons for the N intersections of a surface with each output, each using a different shader? You absolutely do not want to put an if based on a pixel value into a glsl shader. It is really slow. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On 5 April 2013 09:53, Richard Hughes wrote: > * GLib really wants to return all signals (colord_device_changed_cb) > on the main thread, not on the thread that's running the loop. As discussed on IRC, it seems I was wrong. I've attached both patches (v6) for review. I've switched the coldplug onto the cms thread, and switched to using wl_event_loop_add_fd() to batch up the updates. I've got to move onto other stuff next week, but I'd be really interested if anyone has inputs on how the sub-surface gamut mapping using shaders is going to work. The main complication seems to be that it's per-output, rather than per-surface. I've played with 1bit masks in the past, but this made the shader quite complicated. Ideas very welcome. Thanks. Richard 0001-Add-initial-color-management-framework-code.patch Description: Binary data 0002-Add-a-colord-implementation-of-a-CMS-plugin-for-west.patch Description: Binary data ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
Bill Spitzak wrote: These gamma ramps are an attempt to simulate a color conversion from some color space (sRGB?) to the color space of the output device. No, they are not. Gamma ramp setting generally accomplishes one or more of: 1) Setting the brightness to a calibrated level. 2) Setting the white point to a target color temperature. 3) Setting the gray response to a particular curve and neutrality. Although you might be able to accomplish 2) using ICC profiles, it isn't supported very well (absolute colorimetric & V4 ICC profiles etc.), and it's better for the users state of adaptation if the whole screen has a common white point. 3) helps non-color managed application, as well as making the display more easily and accurately color profile-able. So > does this mean that the default color space of the buffers a client renders is sRGB? You can't do that with 1D ramps - you need at least a matrix as well. For general colorspace emulation you need a 3D cLUT. Graeme Gill. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On 4 April 2013 17:11, John Kåre Alsaker wrote: > You should remove the destroy and user_data fields from weston_color_profile. Fixed. > weston_cms_create_profile and weston_cms_load_profile should just > return a pointer to weston_color_profile. Fixed. > Plugins should just exit if compositor->cms is already set in module_init. Fixed. > The call to colord_update_output_from_device in colord_output_added > should probably be done in the GLib thread so it won't block the > compositor. I spent a few hours late last night looking at what could be run in different threads, but two things became clear. * GLib really wants to return all signals (colord_device_changed_cb) on the main thread, not on the thread that's running the loop. * Although the two _sync() calls look scary, I acted on a hunch and added a timer to see how long getting the profile really took: 7ms I think for the sake of not over-complicating things with *hundreds* of lines of extra code, for an event that might only happen once or twice per day or perhaps never at all, blocking the compositor for 7ms is just fine. Changing the screen calibration isn't something that happens frequently at all. I've attached v3 of the patchset, I hope this is okay. Richard. 0001-Add-initial-color-management-framework-code.patch Description: Binary data 0002-Add-a-colord-implementation-of-a-CMS-plugin-for-west.patch Description: Binary data ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On 4 April 2013 21:30, Matthias Clasen wrote: > Just ftr (and without actually looking at the patch): glib signals > don't need an event loop, they're entirely synchronous. Right, but in this case the event is coming from the GDBus thread, which I assume is g_idle_add'ing the signal it back to the mainloop so that it's executed in the main thread. Richard. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On Thu, Apr 4, 2013 at 2:58 AM, John Kåre Alsaker wrote: > I don't see a GLib event loop, so I'm curious to how the signals work. Just ftr (and without actually looking at the patch): glib signals don't need an event loop, they're entirely synchronous. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On 4 April 2013 19:52, Bill Spitzak wrote: > These gamma ramps are an attempt to simulate a color conversion from some > color space (sRGB?) to the color space of the output device. Not really, the gamma ramps just affect the gamma response and the display whitepoint. You can't do color gamut mapping as the curves are 1D. Apple introduced the vcgt calibration tag, and it's been used for a few years mainly for changing the whitepoint. Richard ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
Richard Hughes wrote: On 4 April 2013 19:08, Bill Spitzak wrote: Are clients supposed to tell the compositor what color space their buffer is, and the compositor then converts to the output's color space as part of the compositing step? At the moment I'm just implementing the calibration state setting, i.e. setting the gamma ramps from the VCGT data. When we have sub-surfaces to play with I was going to look at tagging subsurfaces with different color spaces and setting up shaders to do the color conversion to display space, but for the moment I was trying to keep it simple. Okay, it does sound like the compositor is doing the conversion. These gamma ramps are an attempt to simulate a color conversion from some color space (sRGB?) to the color space of the output device. So does this mean that the default color space of the buffers a client renders is sRGB? ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On 4 April 2013 19:08, Bill Spitzak wrote: > Are clients supposed to tell the compositor what color space their buffer > is, and the compositor then converts to the output's color space as part of > the compositing step? At the moment I'm just implementing the calibration state setting, i.e. setting the gamma ramps from the VCGT data. When we have sub-surfaces to play with I was going to look at tagging subsurfaces with different color spaces and setting up shaders to do the color conversion to display space, but for the moment I was trying to keep it simple. Richard. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On 4 April 2013 19:21, Kai-Uwe Behrmann wrote: > Parsing of ICC profiles inside weston is not nice. That would be better > abstracted out to lay inside the CMS plugins. Those CMS plugins can link > against lcms or whatever CMM they prefere. Well, we could certainly add some abstraction to get 3 allocated gamma ramps, although I don't think we should aim to abstract everything without a good reason. It's not likely a different cms will want to load the vcgt calibration data in a different way after all. It's basically table data. > Are you aiming to do spectral imaging in weston? No, that would be silly. Richard. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
Am 03.04.2013 21:47, schrieb Richard Hughes: Patch 1 adds the framework code, patch 2 adds the ability to load CMS modules, and patch 3 adds a CMS module written for colord. With those People have a hard time to understand ICC and colour termininology. So some annotations to that first. A CMS module for a CMF. That is in conflict with your explanations. You defined colord as a CMF but call it a CMS module (one which can do by your definition colour conversions like ColorSync, WCS or Oyranos). But the colord API can not do any colour conversion as would be typical for a CMS. You weston code is outlined as a CMS not a CMF (in your terminology), because you link against a CMM. In case you like to provide the device configuration side only, then lcms is not needed at all. In case colour conversion is part of the plan for weston, then calling this code a CMS is spot on. To the actual code, Parsing of ICC profiles inside weston is not nice. That would be better abstracted out to lay inside the CMS plugins. Those CMS plugins can link against lcms or whatever CMM they prefere. I've left a wodge of documentation in src/cms.h explaining all the terms and the general idea on how I think this should work. There are You explain the term "CMF". The first idea, which pops to my mind, is the "Colour Matching Function", which is fundamental to the CIE and ICC specs. Are you aiming to do spectral imaging in weston? kind regards Kai-Uwe ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
How is this intended to work in Wayland? Are clients supposed to tell the compositor what color space their buffer is, and the compositor then converts to the output's color space as part of the compositing step? Problems with this: * We need a reliable way for clients to tell compositor what color space they are rendering, ideally without huge amounts of code such as the same color space converter library in the client. * Fast conversion of an 8-bit buffer between color spaces reduces the effective number of bits of color to 7 or even 6, and can introduce ugly artifacts. Adding enough noise (what most shaders call dithering) can hide this but obviously adds noise. Computing the output color space at higher accuracy and then doing proper error diffusion dithering is better, but slow on many GPUs. Requiring the buffers to be higher than 8-bit precision can fix this but people may question Wayland's commitment to allowing efficient clients. * Is "same as the output" a supported color space? Or are clients supposed to get the color space needed and convert to it when rendering their buffers? This seems more Wayland-like but has problems: * problem if the surface is on more than one output * multiple copies of the color-correction library and loaded color space descriptions in each client * need for message from compositor to client to change the color space, and compositor must wait for commit from all clients before updating the screen. * Clients may not have access to the color correction hardware that the compositor has. Richard Hughes wrote: I've attached three patches for comments and review. I'm a GLib programmer at heart, so please be kind if I've made huge obvious architectural mistakes :) ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
You can move the output->base.updated_color_profile assignment down to the other of the vfunc initializations. You should remove the destroy and user_data fields from weston_color_profile. weston_cms_create_profile and weston_cms_load_profile should just return a pointer to weston_color_profile. Plugins should just exit if compositor->cms is already set in module_init. The call to colord_update_output_from_device in colord_output_added should probably be done in the GLib thread so it won't block the compositor. colord_update_output_from_device has to execute in the weston thread. I suggest you create a wayland event source and plug that in to the weston event loop. Make a thread safe list of pending output profile updates. When a new update is added trigger the event source. The event source handler should apply all the updates in the list. You should also remove any pending updates for outputs in colord_output_removed. On Thu, Apr 4, 2013 at 5:33 PM, Richard Hughes wrote: > On 4 April 2013 07:58, John Kåre Alsaker wrote: >> The weston_color_manager struct could lose the state field > > All fixed in the newest patchset, thanks for your detailed review. > >> I don't see a GLib event loop, so I'm curious to how the signals work. > > As discussed on IRC, I've used a thread for this. If wayland-glib can > do what we need in the future, I'll switch to using that if the dep is > okay. > > New patches attached. Comments and further review welcomed as usual. Thanks. > > Richard ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
On 4 April 2013 07:58, John Kåre Alsaker wrote: > The weston_color_manager struct could lose the state field All fixed in the newest patchset, thanks for your detailed review. > I don't see a GLib event loop, so I'm curious to how the signals work. As discussed on IRC, I've used a thread for this. If wayland-glib can do what we need in the future, I'll switch to using that if the dep is okay. New patches attached. Comments and further review welcomed as usual. Thanks. Richard 0001-Add-initial-color-management-framework-code.patch Description: Binary data 0002-Add-a-colord-implementation-of-a-CMS-plugin-for-west.patch Description: Binary data ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [patches] Add a color management framework to weston
The weston_color_manager struct could lose the state field. The CMS plugins allocates it and can add state as needed. You should move the weston_cms_output_added call below weston_output_init (which initializes the compositor pointer in the outputs) and drop the first argument from output_added, output_removed, weston_cms_output_added and weston_cms_output_removed. You should make a function which calls set_color_profile in weston instead of calling it directly from the plugins. weston may want to do some non-backend specific processing (such as informing the renderer of the new profile). color_profile has to move from drm_output to weston_output. The struct weston_color_profile * argument can then be dropped from set_color_profile. Perhaps you should rename set_color_profile to updated_color_profile. You should also move the appropriate code from drm_output_set_color_profile into this function. It should be called by the drm backend when using a hardcoded profile. Also allow set_color_profile to be null, which is the case for most backends. I think struct weston_color_profile should also be created in this function with filename and lcms_handle passed as arguments. The CMS plugin should pass ownership of the lcms_handle to weston. I don't see a GLib event loop, so I'm curious to how the signals work. On Wed, Apr 3, 2013 at 9:47 PM, Richard Hughes wrote: > I've attached three patches for comments and review. I'm a GLib > programmer at heart, so please be kind if I've made huge obvious > architectural mistakes :) > > Patch 1 adds the framework code, patch 2 adds the ability to load CMS > modules, and patch 3 adds a CMS module written for colord. With those > three patches it actually works, and you can change the applied > profile at runtime using gnome-color-manager or colord-kde. Note, this > patchset just aims to set the calibration state, but I have left the > lcms hProfile handle available for when we want to construct 3d > shaders for the sub-surface color correction. I've also used all the > new features in colord 0.1.32, but say if you need/want me to tone > this down to something Ubuntu/whatever is shipping. > > I've left a wodge of documentation in src/cms.h explaining all the > terms and the general idea on how I think this should work. There are > a ton of FIXMEs in relation to getting the EDID data, so ideas welcome > there too. Feedback very welcome, thanks. > > Richard. > > ___ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[patches] Add a color management framework to weston
I've attached three patches for comments and review. I'm a GLib programmer at heart, so please be kind if I've made huge obvious architectural mistakes :) Patch 1 adds the framework code, patch 2 adds the ability to load CMS modules, and patch 3 adds a CMS module written for colord. With those three patches it actually works, and you can change the applied profile at runtime using gnome-color-manager or colord-kde. Note, this patchset just aims to set the calibration state, but I have left the lcms hProfile handle available for when we want to construct 3d shaders for the sub-surface color correction. I've also used all the new features in colord 0.1.32, but say if you need/want me to tone this down to something Ubuntu/whatever is shipping. I've left a wodge of documentation in src/cms.h explaining all the terms and the general idea on how I think this should work. There are a ton of FIXMEs in relation to getting the EDID data, so ideas welcome there too. Feedback very welcome, thanks. Richard. 0003-Add-a-colord-implementation-of-a-CMS-plugin-for-west.patch Description: Binary data 0002-Add-the-ability-to-load-CMS-implementations-from-plu.patch Description: Binary data 0001-Add-initial-color-management-framework-code.patch Description: Binary data ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
