Changed filter processing in 23.0.1 ...

JeffTucker

Posts: 8,218
Registered: 31-Jan-2006

Re: Changed filter processing in 23.0.1
Posted: 20 Jul 21, 18:29

in response to: ctwist

Reply

And what can you do about replicative fading? Or is this about something else?

xexyl

Posts: 157
Registered: 1-Sep-2009

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 00:17

in response to: davidekholm

Reply

davidekholm wrote:
Hi. Good that it works for you too. I polished the jalbum-core.jar code a bit so do a core update so you get the most current one. I'm attaching my modification to ReflectionFilter.java here as reference. It now implements Cloneable (which seems to be enough) but also implements the clone method so you can do any customizations to the cloning process, if needed:

    public Object clone() throws CloneNotSupportedException {
        ReflectionFilter clone = (ReflectionFilter)super.clone();
        // Any customizations to the clone here
        //System.out.println("Custom clone");
        return clone;
    }

Just saw this comment. Fortunately it's not needed. I will test the other filters too but I suspect they don't need an update. That will have to be another day as I'm about done for the day.

Thank you.

Let Chris know about this approach. It probably helps him too. If not implementing Cloneable is enough, it should always be possible to get it working by adjusting the cloning process by a custom clone method.

I see he replied (which is why I came back and saw this comment) so I guess I don't need to. But hopefully it worked for him (haven't read his comment yet).

xexyl

Posts: 157
Registered: 1-Sep-2009

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 00:20

in response to: ctwist

Reply

ctwist wrote:
So is cloning the new recommended approach, or is this just a workaround if the previously recommended approach fails?

The way I thought of it it is a workaround but for me it doesn't really matter since it works whereas even having getters/setters didn't seem to solve the problem the other way (though maybe I missed something).

Thank you again for pointing me to this thread as it solved my problem indirectly and I hope it does for you too.

xexyl

Posts: 157
Registered: 1-Sep-2009

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 00:25

in response to: xexyl

Reply

xexyl wrote:

davidekholm wrote:
Hi. Good that it works for you too. I polished the jalbum-core.jar code a bit so do a core update so you get the most current one. I'm attaching my modification to ReflectionFilter.java here as reference. It now implements Cloneable (which seems to be enough) but also implements the clone method so you can do any customizations to the cloning process, if needed:

    public Object clone() throws CloneNotSupportedException {
        ReflectionFilter clone = (ReflectionFilter)super.clone();
        // Any customizations to the clone here
        //System.out.println("Custom clone");
        return clone;
    }

Just saw this comment. Fortunately it's not needed. I will test the other filters too but I suspect they don't need an update. That will have to be another day as I'm about done for the day.

Thank you.

Actually I take that back. Now with ReflectionFilter combined with one of the others it hangs jAlbum. This was not happening before this change you made. Is it possible that implementing the clone() method would solve this? Or is it maybe something else going on in the change you made?

It'll have to be another day that I try that method if you think it'll work but that's disappointment since it was working fine. Please let me know.

Going for the night now.

xexyl

Posts: 157
Registered: 1-Sep-2009

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 00:28

in response to: xexyl

Reply

Actually... I just went to close jAlbum and saw that it threw:

java.lang.OutOfMemoryError: Java heap space

Which didn't happen before. Any idea what might have caused this? Seems strange that implementing clone() would solve it but I also don't know the internals so I'm ignorant on that part.

Edit: Disabling a more advanced use of the filter allows it to work. However the other filter looks strangely wrong as if it isn't working again kind of like what happened with the ReflectionFilter before. Perhaps it does need clone() then? It already implements Cloneable.

Another edit: Implementing clone() doesn't solve the problem and everything is slower since updating the jar file just a short bit ago. Now I really am off for the day.

Edited by: xexyl on 20 Jul 2021, 15:30

Edited by: xexyl on 20 Jul 2021, 15:34

davidekholm

Posts: 3,472
Registered: 18-Oct-2002

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 18:10

in response to: xexyl

Reply

xexyl wrote:

Actually I take that back. Now with ReflectionFilter combined with one of the others it hangs jAlbum. This was not happening before this change you made. Is it possible that implementing the clone() method would solve this? Or is it maybe something else going on in the change you made?

I made no other change. The best way to debug this is to look for any error printouts to jAlbum's system console window (F7). Open it before making the album. If nothing is printed, but jAlbum still hangs, hit the "Dump threads" button of the system console and pass me the generated log file.

davidekholm

Posts: 3,472
Registered: 18-Oct-2002

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 18:15

in response to: ctwist

Reply

ctwist wrote:
So is cloning the new recommended approach, or is this just a workaround if the previously recommended approach fails?

I wouldn't spend time adjusting already working skins, but the now non-working XBorderFilter is a good candidate for this too. Any filters that keeps state variables that aren't pure properties for this as these state variables will also be cloned. Furthermore, if the clone process needs to be more complex, for instance, making a copy of some temporary buffered image, then one can override the clone() method to provide custom cloning. The default clone() method of Object simply does a shallow, field-by-field cloning.

xexyl

Posts: 157
Registered: 1-Sep-2009

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 18:16

in response to: davidekholm

Reply

davidekholm wrote:

xexyl wrote:

Actually I take that back. Now with ReflectionFilter combined with one of the others it hangs jAlbum. This was not happening before this change you made. Is it possible that implementing the clone() method would solve this? Or is it maybe something else going on in the change you made?

I made no other change. The best way to debug this is to look for any error printouts to jAlbum's system console window (F7). Open it before making the album. If nothing is printed, but jAlbum still hangs, hit the "Dump threads" button of the system console and pass me the generated log file.

It's not the filter; it's the jalbum-core.jar update. I didn't use your version of the filter as it wasn't needed and I rolled back to the previous version (the one without the getters/setters). Also it's not the ReflectionFilter that's the problem; it's a different one but one that was working fine until the most recent jar update.

Edit: To clarify the ReflectionFilter does have some pretty exhaustive features BUT it wasn't a problem before and it was only with the other filter - which also wasn't a problem.

Edited by: xexyl on 21 Jul 2021, 09:16

davidekholm

Posts: 3,472
Registered: 18-Oct-2002

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 18:19

in response to: xexyl

Reply

Zip me a project that demonstrates the hanging and I'll check it out. It could be that the cloning needs to be customized further in order to not cause side effects.

xexyl

Posts: 157
Registered: 1-Sep-2009

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 18:25

in response to: davidekholm

Reply

	Elephant.jpg (172.1 KB)
	elephant-phong-bad.jpg (489.7 KB)
	Reflections.jaskin (452.5 KB)

I'm going to first upload a photo with the filter before the most recent jar update and the one after so you can see. If you download my skin and enable the PhongFilter you'll see it looks all funny. That's the easiest way to get it all because like I said it would take quite a bit of work to get it working in the Minimal skin.

Here's the most recent skin and the pictures too. The one that looks all grainy is the bad image. I can upload the source code of the PhongFilter if you want but this was working fine so it has to be something with the most recent jar update.

Note that this is not the best example of the PhongFilter; it's not good for certain type of images but also depending on the settings it can dramatically change the effects.

davidekholm

Posts: 3,472
Registered: 18-Oct-2002

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 19:50

in response to: xexyl

Reply

Please pass me the java source files for the latest version of ReflectionFilter.java AND PhongFilter.java

xexyl

Posts: 157
Registered: 1-Sep-2009

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 20:07

in response to: davidekholm

Reply

Reflections.jaskin (464.1 KB)

Sure. Those aren't the only filters in the skin though if you want the others too? PhongFilter is very complex and there's a lot of comments too. It makes no sense that it would stop working but it stopped working after I updated the jar file.

Anyway here are the filter source files. Actually I'll just attach a new jaskin file with all the filters in it so you can take a look that way if you want.

Thank you.

davidekholm

Posts: 3,472
Registered: 18-Oct-2002

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 21:59

in response to: xexyl

Reply

Reflections.jaskin (491.7 KB)

Thanks. That one was tricky. It turned out that the PhongFilter was modifying several arrays during filter processing, and since arrays are copied by reference instead of being cloned too during the cloning process, all clones of PhongFilter were modifying the same original arrays, thereby overwriting the values for one another.

I resolved the issue by modifying the clone() method of PhongFilter so it also clones all arrays that are created when the original PhongFilter instance is first created. Now each instance is separated properly:

    public Object clone() throws CloneNotSupportedException {
        PhongFilter clone = (PhongFilter)super.clone();
        clone.ambient = ambient.clone();
        clone.diffuse = diffuse.clone();
        clone.specular = specular.clone();
        clone.light = light.clone();
        clone.ambientRGB = ambientRGB.clone();
        clone.diffuseRGB = diffuseRGB.clone();
        clone.specularRGB = specularRGB.clone();
        clone.lightRGB = lightRGB.clone();
        clone.colour = colour.clone();
                
        return clone;
    }

Arrays that are read-only don't need to be cloned, but I didn't want to spend time figuring out which ones are read-only so I cloned all as you can see.

I'm attaching my updated version of your skin here. The speed gain between jAlbum 22 and jAlbum 24.1.1 for building the Sample Portfolio project is huge: From 17s to 10s.

If you have other filters that modify the contents of arrays that are created on object creation, then they need also be cloned like this IF you're implementing the Cloneable interface. If you rely on the JavaBean cloning, then such arrays gets instantiated as a new filter instance is created.

xexyl

Posts: 157
Registered: 1-Sep-2009

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 22:12

in response to: davidekholm

Reply

davidekholm wrote:
Thanks. That one was tricky. It turned out that the PhongFilter was modifying several arrays during filter processing, and since arrays are copied by reference instead of being cloned too during the cloning process, all clones of PhongFilter were modifying the same original arrays, thereby overwriting the values for one another.

Yes it does. That makes sense (or as much sense as my tired head can make of it). Data race I guess is one way of putting it?

I resolved the issue by modifying the clone() method of PhongFilter so it also clones all arrays that are created when the original PhongFilter instance is first created. Now each instance is separated properly:

    public Object clone() throws CloneNotSupportedException {
        PhongFilter clone = (PhongFilter)super.clone();
        clone.ambient = ambient.clone();
        clone.diffuse = diffuse.clone();
        clone.specular = specular.clone();
        clone.light = light.clone();
        clone.ambientRGB = ambientRGB.clone();
        clone.diffuseRGB = diffuseRGB.clone();
        clone.specularRGB = specularRGB.clone();
        clone.lightRGB = lightRGB.clone();
        clone.colour = colour.clone();
                
        return clone;
    }

This solves the problem! Did you modify any of the other filters?

Arrays that are read-only don't need to be cloned, but I didn't want to spend time figuring out which ones are read-only so I cloned all as you can see.

I totally agree that that's the better way to go about it. In fact I couldn't even answer that for you myself (edit: at least without looking at the code for a bit but why bother when it's easiest to do it on all the arrays?).

I'm attaching my updated version of your skin here. The speed gain between jAlbum 22 and jAlbum 24.1.1 for building the Sample Portfolio project is huge: From 17s to 10s.

If you have other filters that modify the contents of arrays that are created on object creation, then they need also be cloned like this IF you're implementing the Cloneable interface. If you rely on the JavaBean cloning, then such arrays gets instantiated as a new filter instance is created.

All seems good - except for the strange combination of three of the filters making the image completely black. But that's I guess to be expected with what the filters do.

Thank you very much! This also solves the problem of slow processing and running out of memory - which makes sense.

Now the one obvious question that remains: do I mark the skin as needing the new jAlbum version or can I keep it as 17?

Have a good night!

Edit: I checked that you didn't modify any other filters (diff) so my sole question remaining is the one on jAlbum version for the skin. Thank you again.

Edited by: xexyl on 21 Jul 2021, 13:22

davidekholm

Posts: 3,472
Registered: 18-Oct-2002

Re: Changed filter processing in 23.0.1
Posted: 21 Jul 21, 22:49

in response to: xexyl

Reply

xexyl wrote:

This solves the problem! Did you modify any of the other filters?

No

All seems good - except for the strange combination of three of the filters making the image completely black. But that's I guess to be expected with what the filters do.

Compare with the result of using jAlbum 22 or jAlbum 24.1.1 with one single processing thread. If there is a difference, then there's more to look into.

Thank you very much! This also solves the problem of slow processing and running out of memory - which makes sense.

Parallel processing can actually use MORE ram, but it's worth it. Due to the fact that jAlbum 24 and 23 clones each filter before applying it and then drops the clones thereafter, then any memory held by the filter is reclaimed faster. This does save RAM.

Now the one obvious question that remains: do I mark the skin as needing the new jAlbum version or can I keep it as 17?

You can keep it at 17 but write a note to users of more recent versions to do a core update to v24.1.1

Have a good night!

Thanks.