Fix for --disable-deprecated (#1273) #1593

kleisauke · 2020-03-24T11:44:05Z

~~Marked this PR as draft since the whole mosaicing folder has yet to be converted to vips8.~~

This PR resolves #1273 and #798.

kleisauke · 2020-04-19T12:47:22Z

This PR is almost done, the whole mosaicing folder was converted to vips8 with commit 2105b76. I'm still working out a few kinks, but feel free to review/comment. Possible further improvements has been preceded by TODO(kleisauke):.

There's now a unit test for vips_mosaic and vips_globalbalance (ported from the 1 point mosaic nip2 demo). Although vips_globalbalance seems to output correctly for int_output=True the float output however seems to be wrong.

I also added a new vips8 class named vips_inversematrix (perhaps it should be named vips_invertmatrix(?)) for inverting square matrices. This class has been converted from im_matinv. The logic for transposing and multiplying matrices are small, so these functions (named vips__transposematrix and vips__multiplymatrix) are included in global_balance.c.

kleisauke · 2020-04-20T10:28:22Z

Although vips_globalbalance seems to output correctly for int_output=True the float output however seems to be wrong.

Fixed with 5b1b8cd.

kleisauke · 2020-04-22T11:56:26Z

Ignore my previous comment, it looks like non-assigned inputs (i.e. input that is not explicitly assigned by the user) are automatically finalized, TIL. So for example the vips_object_local here:

libvips/libvips/resample/resize.c

Lines 267 to 271 in e1baf66

    
           VipsInterpolate *interpolate; 
        
           if( !(interpolate = vips_interpolate_new( nickname )) ) 
        
           	return( -1 );  
        
           vips_object_local( object, interpolate );

Is correct, but not necessary if you do this in vips_affine_build.

jcupitt · 2020-06-18T10:44:18Z

Hello Kleis, yes, I think the verb form is usually better, so invertmatrix.

Actually, we have matrixload, so perhaps matrixinvert might be better?

It's no longer needed after commit bae484b.

Appears to be unused.

From im_matinv

kleisauke · 2020-06-18T13:03:02Z

OK, I cleaned up the commits and renamed it to matrixinvert with kleisauke@45f9999.

This is now ready for reviewing. The mosaic changes are very large, so I think it's wise to fuzz it for a few days before the 8.10 release.

As an aside, I noticed that the test_descriptors with make check is currently failing when building with libspng.

** seq open ..
** crop1, avg ..
** crop2, avg ..
** unref ..
** shutdown ..
lt-test_descriptors: /home/kleisauke/libvips/test/test-suite/images/sample.png: fd not closed after first read
** seq open ..
** crop1, avg ..
FAIL test_descriptors.sh (exit status: 1)

jcupitt · 2020-06-18T14:18:14Z

Ooop, you're fight, it was missing minimise support. It should be fixed now.

Well done on getting all this refactoring sorted out Kleis! I'll try to read it all next week, then perhaps we can aim for July for the 8.10 release.

jcupitt · 2020-06-28T09:46:03Z

libvips/resample/transform.c

@@ -43,26 +43,29 @@
 #include <vips/internal.h>
 #include <vips/transform.h>

+/* DBL_MIN is smallest *normalized* double precision float */
+#define TOO_SMALL 2.0 * DBL_MIN


Should be (2.0 * DBL_MIN)

jcupitt · 2020-06-28T09:57:16Z

libvips/Makefile.am

@@ -24,7 +24,7 @@ SUBDIRS = \
 	iofuncs \
 	morphology \
 	mosaicing \
-	create 
+	create \
 	.


That's weird, I wonder why "." was here? Let's remove this later.

jcupitt · 2020-06-28T10:00:23Z

libvips/iofuncs/init.c

+	/* Older glibs were showing G_LOG_LEVEL_{INFO,DEBUG} messages
+	 * by default
+	 */
+#if GLIB_CHECK_VERSION ( 2, 31, 0 )


jcupitt · 2020-06-28T10:03:47Z

libvips/mosaicing/global_balance.c

 		/*NOTEACHED*/
 	}

 	return( 0 );
 }

+static int
+vips__transposematrix( VipsImage *in, VipsImage **out )


Perhaps should be matrixtranspose, though it doesn't really matter in a static.

jcupitt · 2020-06-28T10:08:06Z

OK, I read the whole thing, nice work! You found a lot of useful small fixes too, and even fixed some spelling.

I wonder how useful some of that old mosaicing stuff is. Nicos wrote most it back in the early 90s. Perhaps it should be moved out of the core library? That's another question I suppose.

Anyway, it looks great, let's merge.

- move matrixinvert to mosaicing, fix a leak - add note to changelog - small fixes see #1593

jcupitt · 2020-06-28T11:17:23Z

I moved matrixinvert to mosaicing (create/ was supposed to just be things that create images), fixed a small memleak, and added a note about your achievement to the main changelog.

kleisauke · 2020-06-28T11:54:20Z

Thanks! The mosaicing stuff could still be useful for museums, but I'm not sure if it's often used. Perhaps Operation Night Watch uses this?

Could the vips_matrixinvert_get_type call also be moved to the vips_mosaicing_operation_init function?

libvips/libvips/create/create.c

Line 172 in e3a5002

vips_matrixinvert_get_type();

The POTFILES.in needs to be regenerated too.

libvips/po/POTFILES.in

Line 212 in 5540fa9

libvips/create/matrixinvert.c

(It should be much easier with these instructions now)

jcupitt · 2020-06-28T13:15:04Z

OK, done!

kleisauke · 2020-06-28T17:31:22Z

I had a go to check the ABI compatibility after this PR, see the abi-compliance-checker result:
https://kleisauke.nl/compat_reports/vips/8.9.2_to_master/compat_report.html

I guess we still have to write some vips7 compat wrappers for the im_correl, im_lrmosaic, im_tbmosaic, im_lrmerge and im_tbmerge functions in deprecated/vips7compat.c? I could pick this up, if you want.

jcupitt · 2020-06-29T02:30:46Z

If you have time, that would be great!

We'll need to make vips_lrmerge() etc. more vips8-y too I think, or there will be API breaks when (if) we eventually make them into classes too. They'll need at least the ..., NULL) parameter ending.

kleisauke · 2020-06-29T12:24:48Z

OK, I started to rework vips_correl as vips8 class, see: master...kleisauke:mosaicing-vips8.

I'm not sure if I should rework im_{lr,tb}mosaic and im_{lr,tb}merge as vips8 classes, since vips_mosaic and vips_merge are already there. Should I try to merge these left-right/top-bottom functions within vips_merge/vips_mosaic? The compat layer can then call up these functions with the appropriate VipsDirection.

jcupitt · 2020-06-29T14:10:45Z

Ah I'd actually forgotten about them, heh. That was a while ago.

Yes, you're right, vips_merge() is the vips8 class interface to the old im_lrmerge() and im_tbmerge(). We should rename your vips_lrmerge() as vips__lrmerge() and have it as internal API. As you say, we could even fold them into merge.c, though that would make the file very large.

Shall I do this?

I wonder if correl is very useful. Perhaps it should just be internal API. If we make it an official part of vips8, we'll have to support the stupid thing for all time.

jcupitt · 2020-06-29T14:13:31Z

Off-topic: I had a go at a first draft of "what's new in 8.10":

https://github.com/libvips/libvips/blob/whatsnew-8.10/_posts/2020-06-18-What's-new-in-8.10.md

Any comments very welcome etc.

kleisauke · 2020-06-30T09:21:03Z

I agree, lets make vips_correl, vips_{lr,tb}mosaic and vips_{lr,tb}merge part of the internal API. vips_correl as a vips8 class will also lead to confusion since there is already a vips_correlation function.

I've already prepared the vips7 compat wrapper functions, see: master...kleisauke:mosaicing-vips8.

ABI compatibility looks OK now, only some internal API symbols have been removed.
https://kleisauke.nl/compat_reports/vips/8.9.2_to_mosaicing-vips8/compat_report.html

Feel free to mark those mosaicing functions as internal, I can do a rebase after that.

kleisauke · 2020-06-30T09:28:57Z

The "what's new in 8.10" news post looks great! The only thing I noticed is that libspng is mentioned twice in the ChangeLog, see:

libvips/ChangeLog

Line 22 in 2b2fabc

- add experimental libspng reader

libvips/ChangeLog

Line 31 in 2b2fabc

- load PNGs with libspng, if possible

jcupitt · 2020-06-30T10:33:10Z

OK, I'll shift them to internal-only. Thanks for the note about the changelog, I've fixed that too.

randy408 · 2020-06-30T16:01:12Z

If the Windows release will default to libspng for loading PNGs it would mean a lot if it was mentioned in the changelog or blogpost.

jcupitt · 2020-07-01T10:46:01Z

Good idea @randy408, I'll add a note.

This comment has been minimized.

Sign in to view

kleisauke added 14 commits June 18, 2020 13:42

Remove deprecated vips7 C++ generator

21239ef

It's no longer needed after commit bae484b.

Remove x.h

9e9c8e6

Appears to be unused.

Disable IM_* environment variables when --disable-deprecated

230d2ac

Avoid using vips7 symbols

8abcae3

Convert the morphology directory to vips8

8a5dc95

Add vips_matrixinvert for inverting matrices

45f9999

From im_matinv

Convert the mosaicing directory to vips8

c3da8ee

Remove outdated comment

597def2

Avoid using unneeded variables

f6b9f38

Add unit test for vips_matrixinvert

4b5db78

Add unit test for vips_mosaic / vips_globalbalance

541e586

Compile with --disable-deprecated on Travis

d67eca7

Update C++ binding and function list

e5323b0

Regenerate POTFILES.in

3836492

kleisauke force-pushed the disable-deprecated branch from b35e05c to 3836492 Compare June 18, 2020 13:00

kleisauke marked this pull request as ready for review June 18, 2020 13:03

kleisauke changed the title ~~[WIP] Fix for --disable-deprecated (#1273)~~ Fix for --disable-deprecated (#1273) Jun 18, 2020

jcupitt reviewed Jun 28, 2020

View reviewed changes

jcupitt merged commit e3a5002 into libvips:master Jun 28, 2020

jcupitt added a commit that referenced this pull request Jun 28, 2020

small cleanups for disable deprecated

5540fa9

- move matrixinvert to mosaicing, fix a leak - add note to changelog - small fixes see #1593

kleisauke deleted the disable-deprecated branch June 28, 2020 11:54

kleisauke mentioned this pull request Mar 3, 2022

Improve symbol visibility #2688

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for --disable-deprecated (#1273) #1593

Fix for --disable-deprecated (#1273) #1593

kleisauke commented Mar 24, 2020 •

edited

kleisauke commented Apr 19, 2020

kleisauke commented Apr 20, 2020

This comment has been minimized.

kleisauke commented Apr 22, 2020

jcupitt commented Jun 18, 2020

kleisauke commented Jun 18, 2020

jcupitt commented Jun 18, 2020

jcupitt Jun 28, 2020

jcupitt Jun 28, 2020

jcupitt Jun 28, 2020

jcupitt Jun 28, 2020

jcupitt commented Jun 28, 2020

jcupitt commented Jun 28, 2020

kleisauke commented Jun 28, 2020

jcupitt commented Jun 28, 2020

kleisauke commented Jun 28, 2020

jcupitt commented Jun 29, 2020

kleisauke commented Jun 29, 2020

jcupitt commented Jun 29, 2020

jcupitt commented Jun 29, 2020

kleisauke commented Jun 30, 2020

kleisauke commented Jun 30, 2020

jcupitt commented Jun 30, 2020

randy408 commented Jun 30, 2020

jcupitt commented Jul 1, 2020

Fix for --disable-deprecated (#1273) #1593

Fix for --disable-deprecated (#1273) #1593

Conversation

kleisauke commented Mar 24, 2020 • edited

kleisauke commented Apr 19, 2020

kleisauke commented Apr 20, 2020

This comment has been minimized.

kleisauke commented Apr 22, 2020

jcupitt commented Jun 18, 2020

kleisauke commented Jun 18, 2020

jcupitt commented Jun 18, 2020

jcupitt Jun 28, 2020

Choose a reason for hiding this comment

jcupitt Jun 28, 2020

Choose a reason for hiding this comment

jcupitt Jun 28, 2020

Choose a reason for hiding this comment

jcupitt Jun 28, 2020

Choose a reason for hiding this comment

jcupitt commented Jun 28, 2020

jcupitt commented Jun 28, 2020

kleisauke commented Jun 28, 2020

jcupitt commented Jun 28, 2020

kleisauke commented Jun 28, 2020

jcupitt commented Jun 29, 2020

kleisauke commented Jun 29, 2020

jcupitt commented Jun 29, 2020

jcupitt commented Jun 29, 2020

kleisauke commented Jun 30, 2020

kleisauke commented Jun 30, 2020

jcupitt commented Jun 30, 2020

randy408 commented Jun 30, 2020

jcupitt commented Jul 1, 2020

kleisauke commented Mar 24, 2020 •

edited