Krita Colorspace and profiles

[Stefano Bonicatti]

> 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.

> 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).

> 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.
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.. 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kimageshop/attachments/20160323/f735b275/attachment.html>