Krita Colorspace and profiles

[Boudewijn Rempt]

On Wed, 23 Mar 2016, Stefano Bonicatti wrote:

> > Some termininology to make sure we're on the same page:

> We are somewhat on the same page, KoColorSpaceFactory has a cache of profiles and colorspace as the KoColorSpaceRegistry do, so the
> latter doesn't only track factories, actually it's job, from what i see, seems mainly to serve as a global colorspace and color profile
> cache.

That is sort of true; at one point I guess we got confused by who owns what, hence the ownedByRegistryRegistryDeletes thing that got added ages ago. I think originally, the factory would own the colorspaces it created while the registry would know about the relations and offer a single point of access to get colorspaces.

> > Because you don't want to specify the profile name all the time if the default for a certain colorspace is fine.

> The problem with accepting the default is that with the fallback code we have, it doesn't really seem to me that you then end up
> getting what you've asked, and if you do.. that's based on several assumptions, not explained, that could be easily broken and do not
> depend on the color space registry itself (aka, you ask for a colorspace with the default profile name, you get the function searching
> through aliases and then creating a colorspace using a profile name coming from neither of the two).

Does that happen? If it would happen, we would have seen bugs before, and I haven't seen those.


> > We shouldn't pre-create KoColorSpace instances for every profile for every colorspace. [...]
> > Not sure what you mean here, either. We load profiles on startup, but we do not (and should not) create colorspaces for all profiles
> on startup.

> Never wanted to create all the colorspaces on startup, quite the opposite, the current behaviour it's more then fine.
> I'm talking about color profiles, which can be (and are) created after the startup through the createColorProfile function.
> The png converter, our KisClipboard are a couple of examples of something that calls that function (and if it finds a duplicate, it
> removes it from the registry and deletes it..., this can be seen in KoColorSpaceFactory::createColorProfile and the respective
> colorProfile call).
> 
> > If you suspect a bug here, we need to check that we indeed never create two colorspaces with the same physical profiles.
> 
> Will do, but given how the locks are done (so not only a profile name issue), that's very possible, since two thread can easily find
> that a certain colorspace isn't in the cache, so they both have to create it.
> Sure, the colorspace map will hold only one of the two, but the other would leak.
> 
> > I'm sure there are problems in KoColorSpaceRegistry, but I'm not sure that this is the real problem, actually. The profile alias code

> is old, but it's used all the time -- what actual problem are you trying to solve in the first place?
> 
> The fact that a code is used and didn't, yet, show issues, doesn't mean it is fine as it is... we are human and prone to error, so
> enforcing some specific behaviour, through code, should be the target, not hoping that people won't use it wrongly.

Of course, but that's some general point that you could make about everything.

> Moreover it's not stated anywhere i could see what are the guarantees or how it should be used to avoid issues, and even then, it's
> much more easier to have the code that controls the user, instead of the user remembering things.
> 
> Then this all started because i was trying to keep colorspaces clean in the leak reports, then got issues with the mixed ownership of
> colorspaces and profiles between the registry and the factories.

Okay, that is a problem. But we need to be really careful when touching this code, it's not a mess because we're too stupid to do it right: it's grown into a tangle because it contains years of bug fixes.

>  and also the fact that the registry own the factories but the
> factories can modify the registry (so who owns who, normally who owns something should be the only been able to modify the owned
> class/data).
> Plus if we want to do lazy profile loading, the current locking is not fine or safe.
> 
> 
>

-- 
Boudewijn Rempt | http://www.krita.org, http://www.valdyas.org