0.3.0 API #10

jballanc · 2016-12-02T14:33:53Z

Updated API for v0.3.0 that allows for efficient single-call decode of byte arrays or files to either a pre-allocated ByteBuffer or returned as a byte array.

The file pointer needs to be reset before initializing the stream. Before this fix, compiling with `DEBUG=1` would lead to a crash on any attempt to transcode files.

With the v0.3.0 API changes, we seem to be hitting the limits of how many directly allocated `ByteBuffer`s one can work with in a single JVM. This moves the largest file first in the test sequence so we can deal with allocation issues sooner.

They eat up memory and make everything sad...

chris-allan · 2016-12-05T15:04:54Z

@melissalinkert should have a look at see what she thinks of the new API. Is there a particular reason the ByteBuffer tests were disabled in dcf512a, @jballanc?

jballanc · 2016-12-05T15:36:46Z

Yes, ByteBuffer tests were failing due to the issues we saw before with creating too many ByteBuffers. I suspect this may have to do with the changes to getImageMetadata, i.e. there's twice the number of ByteBuffers in play because we're no longer assigning the ByteBuffer to an instance and re-using it, but rather passing it as a parameter.

One option would be to change the API specifically for ByteBuffers such that we can instantiate DecodeContext with a ByteBuffer and re-use it, but then the API for ByteBuffer would be different than for File or byte[] which seems less than idea.

I asked @melissalinkert if support for ByteBuffers was critical, and she indicated it was not, so I had intended to simply remove those bits and simplify the API. Working on a proposal for than in a branch of this branch.

chris-allan · 2016-12-05T23:31:40Z

java/src/test/java/ome/jxrlib/TestInMemoryDecode.java

        Assert.assertEquals(md5, md5(destination));
    }

+    /*


Let's add a TestNG annotation here and comment as disabled for now or adjust the test cases to re-use a ByteBuffer if it's a memory issue we're running into.

chris-allan · 2016-12-05T23:32:16Z

java/src/test/resources/testng.xml

@@ -3,6 +3,19 @@

 <suite name="ome.jxrlib">

+  <test name="FL_3channel_noZ_compression_lossless">


Is there a particular reason this was moved up?

It's the largest of the test files, so moving it was moved up in the hope that (combined with the buffer reallocation workaround in e29028c) running it first would avoid the issues with ByteBuffers. (This worked on my machine, but apparently not on Travis.)

chris-allan · 2016-12-05T23:35:13Z

I can't see a reason not to just make it work by adjusting the test cases. We know the size of the required buffers as well based on the test case fixture data being passed in and we can pre-allocate, pass in and/or reuse the buffer as required by setting the TestNG scope.

jballanc · 2016-12-07T15:03:40Z

We could certainly re-jigger the tests to avoid excessive ByteBuffer allocation. I already attempted a janky solution along these lines in e29028c, but apparently to no avail. If keeping the ByteBuffer APIs around is a priority, I can look at this again, but my impression was that those APIs were work-arounds until direct decoding of byte[] was possible (which it is now), and so they aren't really needed any longer.

Or, rather, it's not necessary to use ByteBuffers with DecodeContext. We could certainly keep the ByteBuffer API in ImageDecode and CodecFactory, if desired, for those that want to separate ImageDecode creation and image decoding. In that case, though, we should write tests that exercise those APIs directly (and avoid more ByteBuffers than necessary).

chris-allan · 2016-12-07T18:46:59Z

They're not really a work around but rather an alternative.

The first thing I would try to allocate the buffers ahead of time in the test case itself and utilise TestNG scoping to ensure that they are not destroyed.

This commit includes the Doxygen configuration, a `make` target to generate docs, and documentation for most of the classes.

This commit includes documentation comments for `ImageEncoder`, `ImageMetadata`, `Resolution`, and `Stream`.

For whatever reason it seems that passing object references causes crashes when using Java `ByteBuffer`s to pass image data. Reverting this back to pass pointers.

This reverts commit e29028c.

This reverts commit fe6b4dd.

This reverts commit dcf512a.

chris-allan · 2017-01-23T14:19:04Z

Two things, @jballanc:

Lets get some test cases added for decoding into a larger ByteBuffer at a given offset
We should probably have getImageMetadata(ByteBuffer source) added to the API and tests added for it

This reverts commit f16c98d.

melissalinkert · 2017-01-28T18:46:03Z

Java API looks good in general. I could see it being useful to have something like AbstractDecode.decodeFrame(int frame, byte[] source, int sourceOffset, int sourceLength, byte[] destination, int destinationOffset), especially for consistency with the other decodeFrame signatures that take a File or ByteBuffer. It might also be helpful to have place-holder getFrameCount methods that just returns 1 - we know that there is only one frame in practice, but that won't be obvious to anyone external. Adding a frame count field to ImageMetadata would also work, if that's easier.

Otherwise, I think it would be good to do a little more bounds checking in the decodeFrame methods, to ensure that the frame and offset are non-negative and the length is positive.

This is the first step of refactoring the C++ and Java APIs so that they are more consistent. On the C++ side, input buffers are always `unsigned char` and instead of multiple overloads, optional offsets are represented as optional parameters. This does necessitate rearranging parameter order on some methods, so all methods were re-arranged so that offsets and lengths appear at the end of the parameter list. On the Java side, instead of relying on the built-in SWIG mechanisms for passing NIO Buffers and `byte[]` across the JNI bridge, we have copied the definitions into our `JXR.i` definitions and changed the `byte[]` implementation to pass across as `unsigned char *`. This allows us to use the same C++ methods for both, though due to limitations with SWIG auto-generation, we need to use the `%rename` directive to prevent definitions from replacing each other. This results in a new set of more descriptive method names.

This change brings the header doc comments back in line with the actual parameter lists for DecodeContext's methods that were changed in the last commit.

With this set of changes, we should have the final version of the API that we want for AbstractDecode, including congruent methods to decode either `byte[]`s or `ByteBuffer`s with an offset and length.

These methods gave the API just a bit *too much* surface area. For now, if a user needs a `byte[]` from the decoded image, they can use the methods that return `byte[]` and if they need to write into an array at an offset they can use the methods that write to `ByteBuffer`s. (Really, we just don't have sufficient testing around passing `byte[]`s across the SWIG/JNI bridge as out parameters.)

With the finished `AbstractDecode`/`Decode` API, we no longer need mappings to the lower-level C++ APIs. Not to mention that, due to issues with pointer lifetimes, we cannot guarantee their use from Java is safe.

melissalinkert · 2024-03-05T21:40:23Z

Closing as discussed in the context of upcoming work on #32.

chris-allan and others added 8 commits November 18, 2016 16:48

Initial version of 0.3.0 API

89a90ae

Implement new methods for v0.3.0 API

6b5192a

Move debug-compensating fseek

c4b2be1

The file pointer needs to be reset before initializing the stream. Before this fix, compiling with `DEBUG=1` would lead to a crash on any attempt to transcode files.

Get the correct full file URL for test files

c725706

Try testing the big file first

389e344

With the v0.3.0 API changes, we seem to be hitting the limits of how many directly allocated `ByteBuffer`s one can work with in a single JVM. This moves the largest file first in the test sequence so we can deal with allocation issues sooner.

Recycle ByteBuffer instances in Java tests

e29028c

Comment out tests checking non-direct-allocated ByteBuffers

fe6b4dd

Disable all ByteBuffer tests

dcf512a

They eat up memory and make everything sad...

chris-allan reviewed Dec 5, 2016

View reviewed changes

jballanc and others added 10 commits January 5, 2017 10:02

Add documentation for C++ classes

2cb2fa4

This commit includes the Doxygen configuration, a `make` target to generate docs, and documentation for most of the classes.

Fix name of PCRE lib for Travis

93d7d3d

Add documentation for remaining C++ classes

59b35d6

This commit includes documentation comments for `ImageEncoder`, `ImageMetadata`, `Resolution`, and `Stream`.

Revert back to passing ImageDecoder pointers

f16c98d

For whatever reason it seems that passing object references causes crashes when using Java `ByteBuffer`s to pass image data. Reverting this back to pass pointers.

Revert "Recycle ByteBuffer instances in Java tests"

ba97021

This reverts commit e29028c.

Revert "Comment out tests checking non-direct-allocated ByteBuffers"

a9edef8

This reverts commit fe6b4dd.

Revert "Disable all ByteBuffer tests"

e12b4b0

This reverts commit dcf512a.

Ensure wrapping with a Java exception handler

b0eb4ac

Fix offset typo

33e41fa

Fix invalid input test cases

317c511

chris-allan force-pushed the 0.3.0-API branch from 4c4c0c2 to 317c511 Compare January 20, 2017 17:41

jballanc and others added 4 commits January 20, 2017 12:45

Fix buffer length parameter typo

7205ce5

Push crash logs if they exist

27dcbd9

Clean up long lines

a9467c6

Fix additional buffer length parameter typo

cf86193

chris-allan and others added 4 commits January 23, 2017 06:33

Clean up long lines

8e931df

Revert "Revert back to passing ImageDecoder pointers"

eb5d7ee

This reverts commit f16c98d.

Add an implementation of getImageMetadata for ByteBuffers

2774f1e

Add more checks on offset and length arguments

cffba3d

jballanc added 7 commits February 8, 2017 10:38

Rectify documentation with revised method signatures

68ac6ac

This change brings the header doc comments back in line with the actual parameter lists for DecodeContext's methods that were changed in the last commit.

Add frameCount to ImageMetadata

6369932

Update AbstractDecode Java class to reflect refactored C++ API

b64c2eb

With this set of changes, we should have the final version of the API that we want for AbstractDecode, including congruent methods to decode either `byte[]`s or `ByteBuffer`s with an offset and length.

Remove un-used classes from SWIG

df5fcc5

With the finished `AbstractDecode`/`Decode` API, we no longer need mappings to the lower-level C++ APIs. Not to mention that, due to issues with pointer lifetimes, we cannot guarantee their use from Java is safe.

Update Travis build instructions for new macOS VM image

fbde15e

jballanc force-pushed the 0.3.0-API branch from ac19ddd to 5a097b7 Compare February 9, 2017 18:17

Also install SWIG on macOS via Homebrew

dc8392a

jballanc force-pushed the 0.3.0-API branch from 5a097b7 to dc8392a Compare February 9, 2017 23:39

Ensure that the HOMEBREW option is set for Travis macOS builds

560f60f

jballanc force-pushed the 0.3.0-API branch from da7b728 to 560f60f Compare February 15, 2017 14:52

jballanc and others added 5 commits February 15, 2017 10:32

Attempt to force OpenSSL installation on Travis

d5d0f36

Modify the build.gradle to use the OME Maven and staging repositories

7b9edf6

Use the correct terminology for Maven repositories

ac320ce

Explicitly set xcode version

ef48836

Update Travis build instructions for new macOS VM image

2977585

melissalinkert closed this Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

0.3.0 API #10

0.3.0 API #10

jballanc commented Dec 2, 2016

chris-allan commented Dec 5, 2016

jballanc commented Dec 5, 2016

chris-allan Dec 5, 2016

chris-allan Dec 5, 2016

jballanc Dec 7, 2016

chris-allan commented Dec 5, 2016

jballanc commented Dec 7, 2016

chris-allan commented Dec 7, 2016

chris-allan commented Jan 23, 2017

melissalinkert commented Jan 28, 2017

melissalinkert commented Mar 5, 2024

		@@ -3,6 +3,19 @@

		<suite name="ome.jxrlib">

		<test name="FL_3channel_noZ_compression_lossless">

0.3.0 API #10

0.3.0 API #10

Conversation

jballanc commented Dec 2, 2016

chris-allan commented Dec 5, 2016

jballanc commented Dec 5, 2016

chris-allan Dec 5, 2016

Choose a reason for hiding this comment

chris-allan Dec 5, 2016

Choose a reason for hiding this comment

jballanc Dec 7, 2016

Choose a reason for hiding this comment

chris-allan commented Dec 5, 2016

jballanc commented Dec 7, 2016

chris-allan commented Dec 7, 2016

chris-allan commented Jan 23, 2017

melissalinkert commented Jan 28, 2017

melissalinkert commented Mar 5, 2024