search

leduycuong86 / javacv

Automatically exported from code.google.com/p/javacv
GNU General Public License v2.0
0 stars 0 forks source link

Cached value of CvMat.fullSize and *Buffer fields might get incorrect #317

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Consider the following code:

  CvMat mat = CvMat.createHeader(10, 10, CV_8U);
  cvGetSubRect(largerImage, mat, cvRect(100, 100, 10, 10));

Now mat.getByteBuffer() returns a buffer with limit=100, while it should be 
close to largerImage.fullSize, as mat now refers to the data of a larger image.

To work the problem around call reset() before calling getByteBuffer().

I use JavaCV 0.5.

I think you should not cache fullSize inside of CvMat at all, as you never 
know, when it changes. Various oprations reallocate data, if the target mat 
does not fit the expected dimensions/type, which in turn changes the fullSize.

Original issue reported on code.google.com by viliam.d...@gmail.com on 13 May 2013 at 11:25

GoogleCodeExporter commented 9 years ago 
I ran your test on my machine. Here are the results:

1. HotSpot 1.7.0_17 64-bit
2. HotSpot 1.6.0_41 64-bit
3. Android 4.4 nexus 7 2012

        (1)     (2)     (3)
Array   270     270     412
Buffer  290     240    2262

So not so big difference on HotSpot. Apparently, we are in the area of 
microoptimisation, and it seems very dependent on runtime configuration. As we 
saw earlier, caching indexer is only useful on Android (or maybe some less 
performant VMs too), and the third result decides it (I only used 5MB array, 
because of memory constraints).

So for this moment, I think, that the version from the last patch is best so 
far...

Original comment by viliam.d...@gmail.com on 22 Dec 2013 at 10:04

GoogleCodeExporter commented 9 years ago 
I will not have access to my development computer until January 7th, but I will 
be able to reply. I wish you Merry Christmas and a happy new year :). I'm glad 
for this cooperation, we will be definitely a lot better than the current API...

Original comment by viliam.d...@gmail.com on 22 Dec 2013 at 10:06

GoogleCodeExporter commented 9 years ago 
Thanks for trying it out! Yes, I understand that you may not always have access 
to your machines, that's fine...

So, it looks like Android doesn't like it that way. Let's think about this a 
bit more. I'm not a big fan of overengineering... Merry Christmas!

Original comment by samuel.a...@gmail.com on 23 Dec 2013 at 3:02

GoogleCodeExporter commented 9 years ago 
I think I see the problem with Android. According to "Ben, one of the engineers 
working on the JIT @ Google":
http://stackoverflow.com/questions/4912695/what-optimizations-can-i-expect-from-
dalvik-and-the-android-toolchain
Dalvik can only inline methods that do not have branches, but ByteBuffer.get() 
does branch in checkIndex():
https://android.googlesource.com/platform/libcore/+/master/luni/src/main/java/ja
va/nio/ByteArrayBuffer.java
So it looks like we can't use NIO buffers at all if we're looking into having 
high-ish performance on Android :(

Original comment by samuel.a...@gmail.com on 23 Dec 2013 at 3:19

GoogleCodeExporter commented 9 years ago 
Happy New Year! So, any news on your part?

I have a couple of new ideas about how the API should look like, so I'm jotting 
them down here. For consistency, I think we should mimic how things are done 
with Buffer and in C/C++. In the case of ByteIndexer, for example:

ByteIndexer wrap(BytePointer pointer, int ... sizes)
ByteIndexer wrap(ByteBuffer buffer, int ... sizes)
ByteIndexer wrap(byte[] array, int offset, int ... sizes)
byte get(int[] indices)
ByteIndexer put(int[] indices, byte b)
along with optimized get() and put() for 1, 2, and 3 dimensions

I don't think we'd need to provide a factory method for "steps" because that's 
not how it works with multidimensional arrays in C/C++ anyway, and AFAIK not 
even OpenCV ever aligns rows that badly on purpose, or does it?

What do you think?

Original comment by samuel.a...@gmail.com on 11 Jan 2014 at 10:48

GoogleCodeExporter commented 9 years ago 
Hello,

regarding the name, I don't mind, but I personally prefer "create" version,
as this is commonly used in Java for factory methods. On Wikipedia they use
"new" prefix. "Wrap" evokes the delegate pattern, which is not exactly this
case.

Secondly, OpenCV uses non-continuous arrays for submats (cvGetSubRect), so
we need the "steps" version.

I will add the array version of get/put probably tomorrow.
 D�a 11.1.2014 11:48 pou��vate� <javacv@googlecode.com> nap�sal:

Original comment by viliam.d...@gmail.com on 13 Jan 2014 at 5:54

GoogleCodeExporter commented 9 years ago 
Thanks, but no need to update the source right away!

The problem with "create" is that we need to put something after, while "wrap" 
is already a name used by ByteBuffer, etc to do exactly the same thing (that 
is, add missing functionality from the wrapped object). I've already done that 
for capacity(), position(), and limit() for example and it's working just fine, 
so IMO we might as well keep doing the same thing :)

AFAIK, cvGetSubRect() is never going to cut in the middle of a pixel, so even 
if the start position changes, the effective "width" isn't going to change. 
Unless we keep two sets of variables? Could you elaborate on how is this could 
work for 4, or even 10 dimensional access?

Original comment by samuel.a...@gmail.com on 14 Jan 2014 at 12:48

GoogleCodeExporter commented 9 years ago 
Wait a minute, when creating a 24-bit per pixel RGB image with a width of 1001, 
OpenCV returns an image with a "widthStep" of 3004 bytes: Ouch.

So, I guess your proposal is to base everything on steps and not on dimensional 
sizes?

Original comment by samuel.a...@gmail.com on 17 Jan 2014 at 1:21

GoogleCodeExporter commented 9 years ago 
Sorry for responding late.

We need to keep the "steps" version, and we can decide on keeping the 
"dimensions" one. I think the latter it is simpler to understand for javists, 
who do not commonly use array padding or store multidimensional arrays in 1D 
ones. But, on the other side, if someone will try to create indexer for a CvMat 
without understanding it's internal storage, trying to provide 
rows/cols/channels as dimensions (which he understands) instead of steps (which 
he doesn't) will lead to more confusion. So I don't know whether to keep it or 
remove it for the same reason of simplicity. As of the rule "less code is 
better", I would remove it, it will force people to learn the necessary 
internals or use the CvMat.createXxxPixelIndexer.

And all our indexers should only be created from Pointers. There are already 
methods to convert bytes/pointers/arrays from/to each other. Our utilities are 
supposed to be used with JavaCPP classes, which always converts pointers to 
buffers and not vice versa and never uses java arrays. The other methods would 
only be useful, if indexers are used as a general purpose utility for data 
coming from somewhere else, which I don't expect. I think, this would be 
overengineering and not really needed functionality.

Original comment by viliam.d...@gmail.com on 20 Jan 2014 at 6:40

GoogleCodeExporter commented 9 years ago 
Yeah, we'll go with steps. Although we might want to name it "stride" because 
that's what they use in Java:
http://docs.oracle.com/javafx/2/api/javafx/scene/image/PixelReader.html
http://docs.oracle.com/javase/7/docs/api/java/awt/image/Raster.html

I wouldn't consider it overengineering, because we have to use a Java array in 
the case of Android. And actually I think it would simplifying things. We'd 
have ByteBufferIndexer that does its thing with ByteBuffer (created from a 
Pointer or not), and ByteArrayIndexer, that would accept an array, or create 
one from a Pointer. This way, the user wouldn't have to consider 
ByteArrayIndexer as some sort of "cache" or "copy". Conceptually it would just 
be doing something with an array, which it got from a Pointer maybe. And we 
could have easy-to-understand ByteArrayIndexer.readPointer()/writePointer() or 
something that would do exactly what one would expect! Sounds nice to me. What 
about you?

Original comment by samuel.a...@gmail.com on 24 Jan 2014 at 8:20

GoogleCodeExporter commented 9 years ago 
The array thing is only to solve poor performance of direct ByteBuffer on 
Android. While working with data on native heap, we should normally access it 
via Buffer-s, that should be in principle the fastest. But due to poor 
implementation on android, we have to hack it with an array, and not only that, 
we have to get that array via Pointer. For the user, the change is only in one 
boolean parameter, that could be stored in one global constant and changed 
appropriately.

I still disagree with making this a general purpose utility for accessing data 
of this structure, for few reasons:
- CV applications are usually performance constrained. For this, usually the 
most direct method is the fastest. In our case, even using indexer is bit 
slower than directly using buffers.
- It is trivial to write utility such as this one, if one understands the data 
structure.
- No one will search for it here, JavaCPP or JavaCV is not a general purpose 
utility package. If such package was already present in OpenCV, then this does 
not hold.
- It's purpose is mainly to provide convenience method for accessing elements 
of CvMat, as a faster replacement of now deprecated methods of CvMat. 

Similarly, I don't agree with "stride". We are not translating OpenCV to Java 
terminology. OpenCV uses "step", so should we. For this reason, I would even 
rename the "createBytePixelIndexer" to "createByteDataIndexer" in the CvMat 
class, because it is internally named "data", though in most cases is's 
"pixels". We should stick to simple mapping.

Maybe we could add the Indexer factory to another JavaCpp-ported libraries, 
which have similar structure of data. If it is not consistently named "steps" 
here, then I will agree with using "stride" within java.

But you have the last word.

Original comment by viliam.d...@gmail.com on 27 Jan 2014 at 6:28

GoogleCodeExporter commented 9 years ago 
Let's take a step back here. My intention with whatever goes in JavaCPP is to 
have it available to more than just OpenCV. Of course it's "trivial", but it 
happens that many people like to use MATLAB because it offers that kind of 
"trivial" functionality. Even Scala provides a feature for that:
http://www.tutorialspoint.com/scala/scala_arrays.htm

If someone were to add that to some other package, that would be nice. 
Unfortunately, it's not happening, so I thought I might as well offer it as 
part of JavaCPP, instead of only JavaCV, that's all. And if one were to happen 
to use some other library in Java that uses arrays (because that's the whole 
point of using Java, we're not limited to OpenCV), then that user could use 
those facilities to exchange data more easily between whatever other library 
one wishes to use with OpenCV, such as JavaFX, for example.

BTW, OpenCV *does* already offer that kind of "trivial" functionality, but JNI 
cannot yet inline native functions, so if we want performance, we need to rely 
on direct NIO buffers, or on arrays in the case of Android.

There must be some better name for "stride", "step", or "pitch"... Wikipedia 
doesn't offer much insight I find:
http://en.wikipedia.org/wiki/Stride_of_an_array

Original comment by samuel.a...@gmail.com on 27 Jan 2014 at 8:34

GoogleCodeExporter commented 9 years ago 
What do you think of this API?:

package com.googlecode.javacpp.indexers;

import java.nio.ByteBuffer;

import com.googlecode.javacpp.BytePointer;

/**
 * Convenient way to access elements of an signed 8-bit multidimensional array.
 * 
 * <p>Indexerss are available for all java primitive types.
 * 
 * @author Viliam Durina
 */
public abstract class ByteIndexer {

    protected final int[] strides;

    public ByteIndexer(int[] strides) {
        this.strides = strides;
    }

    /** Get element [i, j] of multidimensional array */
    public abstract byte get(int i, int j);
    /** Get element [i, j, k] of multidimensional array */
    public abstract byte get(int i, int j, int k);
    /** Get element at specified coordinates of multidimensional array */
    public abstract byte get(int ... coords);
    /** Set element [i, j] of multidimensional array */
    public abstract void put(byte v, int i, int j);
    /** Set element [i, j, k] of multidimensional array */
    public abstract void put(byte v, int i, int j, int k);
    /** Set element at specified coordinates of multidimensional array */
    public abstract void put(byte v, int ... coords);
    /** Copies modification back to the native memory after processing is done. No-op, if not cached. */
    public abstract void flushCache();
    /** Discards modifications to the cached copy and populates cache anew. No need to call this after creation. No-op, if not cached */
    public abstract void fillCache();

    /**
     * Factory method.
     * 
     * @param pointer The native pointer. It should have the {@link BytePointer#capacity(int)} set prior to the call.
     * 
     * @param strides <code>strides[0]</code> - byte position of the [1, 0, 0]th element,
     *     <code>strides[1]</code> - byte position of the [0, 1, 0]th elemnts etc.
     *     If the array is contiguous, see {@link #calcContiguousStrides(int[])}
     *     See <a href='http://en.wikipedia.org/wiki/Stride_of_an_array'>Wikipedia/Stride of an array</a>.
     * 
     * @param cached Whether to copy the buffer (this gives faster access on Android devices, tested up to android 4.4,
     *      but do your own test)
     * 
     * @return Initialized instance
     */
    public static ByteIndexer wrap(BytePointer pointer, int[] strides, boolean cached) {
        if (cached)
            return new CachingByteIndexer(pointer, strides);
        else
            return new DirectByteIndexer(pointer.asBuffer(), strides);
    }

    /**
     * @see #wrapNonUnit(ByteBuffer, int[], boolean)
     */
    public static ByteIndexer wrap(ByteBuffer buffer, int[] strides, boolean cached) {
        if (cached)
            return new CachingByteIndexer(new BytePointer(buffer), strides);
        else
            return new DirectByteIndexer(buffer, strides);
    }

    /**
     * @see #wrapNonUnit(ByteBuffer, int[], boolean)
     */
    public static ByteIndexer wrap(byte[] data, int[] strides, boolean cached) {
        if (cached)
            throw new UnsupportedOperationException("We don't support caching of java arrays");
        else
            return new CachingByteIndexer(data, strides);
    }

    /**
     * Calculates stride values for supplied dimensions for contiguous array.
     *
     * See <a href='http://en.wikipedia.org/wiki/Stride_of_an_array'>Wikipedia/Stride of an array</a>.
     */
    public static int[] calcContiguousStrides(int[] dims) {
        int strides[] = new int [dims.length];
        strides[dims.length -1] = 1;
        for (int i=dims.length - 2; i>=0; i--)
            strides[i] = dims[i] * strides[i+1];

        return strides;
    }

}

It has "wrap" method for strides. I removed the createForDims factory method, 
and replaced it with calcContiguousStrides, which calculates strides from 
dimensions. I also gave the link to the wikipedia page, which I find useful for 
those, who are not familiar with "strides".

I don't like the wrap(byte[]) method. I throw UnsupportedOperationException 
there, if caching is requested. For caching to work (if we want it), I would 
need to crate a third ByteIndexer subclass that will clone the array and be 
able to copy back. Or complicate the code of CachingByteIndexer to accept array 
which it will clone. Do we really want that?

Original comment by viliam.d...@gmail.com on 28 Jan 2014 at 7:01

GoogleCodeExporter commented 9 years ago 
Well, that still looks like a poor hack... As I explained a few times already, 
I'd like to put something a bit more generally useful in JavaCPP. You said that 
"even using indexer is bit slower than directly using buffers", but this isn't 
even true. With a properly optimizing compiler, everything can gets inlined and 
moved around appropriately. Here's proof that OpenJDK 7 does that already. This 
is the output of the attached "TestIndexer2" on my machine:
    indexer: 3151 ms
    array: 3152 ms
This shows that it doesn't need to be slower than directly using arrays or 
buffers.

Now, about the API, I guess we should try to make it similar to C# since they 
already have something like that in there:
    http://msdn.microsoft.com/en-us/library/system.array.createinstance
So, naming the factory method "create" might not be such a bad idea, but then 
again in our case we don't allocate the data, so "wrap" might still be a 
better... Also, we note that the user provides the dimensions of array, not the 
strides. The runtime should by default allocate whatever is most efficient for 
the machine, but we must also provide an additional optional "stride" 
parameter, just like it's possible with OpenCV. Consequently, the factory 
methods might look something like 
    ByteIndexer.wrap(byte[] array, int ... sizes)
    ByteIndexer.wrap(ByteBuffer buffer, int ... sizes)
    ByteIndexer.wrap(byte[] array, int[] sizes, int[] strides)
    ByteIndexer.wrap(ByteBuffer buffer, int[] sizes int[] strides)
And the accessors:
    byte get(int ... indices)
    ByteIndexer put(int i, byte b)
    ByteIndexer put(int i, int j, byte b)
    ByteIndexer put(int i, int j, int k, byte b)
    ByteIndexer put(int[] indices, byte b)
With parameters ordered to be consistent with both arrays and NIO buffers. 
Along with a couple of utility methods like read(Pointer)/write(Pointer) I 
guess.

That's starting to sound like something I want to use. Do you feel differently? 
Do you still feel like this would not be generally useful functionality?

Original comment by samuel.a...@gmail.com on 3 Feb 2014 at 1:06

GoogleCodeExporter commented 9 years ago

Original comment by samuel.a...@gmail.com on 3 Feb 2014 at 1:08

Attachments:

GoogleCodeExporter commented 9 years ago 
First to the benchmark: I was bit surprised with your results, but now it seems 
logical to me. I even tried to rewrite the array version to something I thought 
should be faster, but actually, it was a bit slower than before:

    static long testArray(byte[] array) {
        long sum = 0;
        for (int i = 0, row = 0; i < SIZE; i++, row += SIZE) {
            for (int j = 0, idx = row; j < SIZE; j++, idx++) {
                sum += array[idx];
            }
        }
        return sum;
    }

I'm rather confused of this micro-optimisation. In my previous tests, the 
indexer version was really slower, look at comment 34, test code is there. But 
not always :(. Maybe it was because of base class and subclasses and virtual 
methods. Definitely, direct access is simpler for compiler to optimize, even 
though it might be able to optimize the indexer version to the same performance.

But leave that for now, we will include indexer in any case. Let's propose the 
API. I don't understand the method 
    ByteIndexer.wrap(byte[] array, int[] sizes, int[] strides)

Why do we need both sizes and strides? Internally, we will only use strides. If 
we did range checking, it would only make things slower. 

And should we not support automatic copying and leave that to the user? That 
would remove the need to have base class and subclasses, but the code would be 
more complicated, and does it have performance benefit on android to omit it?

So please finalize the API, and I will implement it. I'd wish to finally finish 
this neverending discussion :)

Original comment by viliam.d...@gmail.com on 4 Feb 2014 at 5:38

GoogleCodeExporter commented 9 years ago 
Sorry for the delay. Been busy with other stuff, check it out BTW:
    http://code.google.com/p/javacpp/wiki/Presets
And forgot to reply.

Regarding your comment #34, "sumBuffer" and "sumIndexer" are pretty much the 
equal, and the same for "addBuffer" and "addIndexer" so I'm not sure what you 
are surprised about. Of course it's a bit more difficult for a compiler to 
optimize virtual calls, but Hot Spot does it just fine.

Some code that uses an `Indexer` might need to know the sizes to perform some 
operation. So we'd need to provide some `int[] sizes()` method, and also a 
`int[] strides()` one too I guess.

What do you mean by "automatic copying"? When would the data get copied? AFAIU, 
this requires manual intervention, however we look at it. Since we have to do 
it manually anyway, we might as well make the interface more general, that's 
all. If we can do it automatically somehow, then that's a different matter, so 
please do explain.

Yeah, I agree, it would be nice to be able do this work in person :)

Original comment by samuel.a...@gmail.com on 2 Mar 2014 at 5:05

GoogleCodeExporter commented 9 years ago 
Unless you have some news concerning the support for the "automatic copying" 
you were mentioning, I think I've finally come up with a design that fully 
satisfies me! Basically, I thought of having an interface Indexable similar to 
Iterable, where the Indexable/Indexer pair has similar interactions to the 
Iterable/Iterator one. This way, the Indexer code needs to know nothing about 
Pointer, and we can copy the data around in a much more natural way.

public abstract class Indexer {
    protected int[] sizes;
    protected int[] strides;

    public int[] sizes() { return sizes; }
    public int[] strides() { return strides; }

    public abstract Object array();
    public abstract Buffer buffer();
}

public abstract class ByteIndexer extends Indexer {
    public ByteIndexer wrap(ByteBuffer buffer, int ... sizes) { ... }
    public ByteIndexer wrap(byte[] array, int offset, int ... sizes) { ... }
    public ByteIndexer wrap(ByteBuffer buffer, int[] sizes, int[] strides) { ... }
    public ByteIndexer wrap(byte[] array, int offset, int[] sizes, int[] strides) { ... }

    public abstract byte get(int i);
    public abstract byte get(int i, int j);
    public abstract byte get(int i, int j, int k);
    public abstract byte get(int ... indices);

    public abstract ByteIndexer put(int i, byte b);
    public abstract ByteIndexer put(int i, int j, byte b);
    public abstract ByteIndexer put(int i, int j, int k, byte b);
    public abstract ByteIndexer put(int[] indices, byte b);
}

class　ByteBufferIndexer extends ByteIndexer { ... }

class ByteArrayIndexer extends ByteIndexer { ... }

public interface Indexable {
    <I extends Indexer> I createIndexer(boolean direct);
    void readIndexer(Indexer i);
    void writeIndexer(Indexer i);
}

With that, the class the implements the interface does what it needs to do to 
return the appropriate Indexer. readIndexer()/writeIndexer() would do the 
copying around, but could be implemented as a noop when it happens that the 
Indexable and the Indexer both point to the same area of memory.

Of course we're going to need to do some benchmarking to make sure we don't 
sacrifice any performance with this class hierarchy... 

Concretely, I'm thinking of having the AbstractMat here implement this:
http://code.google.com/p/javacpp/source/browse/opencv/src/main/java/com/googleco
de/javacpp/helper/opencv_core.java?repo=presets#2188
And maybe eventually apply that to AbstractCvMat as well, but we could simply 
leave the old interface as deprecated since OpenCV itself has deprecated CvMat.

In any case, please let me know what you think! If it's something you'd like to 
work on, please let me know. (If not, I will be implementing something like 
that myself eventually.) Thanks!

Original comment by samuel.a...@gmail.com on 16 Mar 2014 at 6:36

GoogleCodeExporter commented 9 years ago 
That said, since the get()/put() methods of an Indexer doesn't change its 
state, unlike the next()/remove() methods of an Iterator, we might want to have 
our Indexable reuse the Indexer object. So, something like this may make more 
sense:

public interface Indexable {
    <I extends Indexer> I acquireIndexer(boolean direct);
    void releaseIndexer(Indexer i);
}

Where acquireIndexer() would read out data, and releaseIndexer() would write it 
back out (both being noops for direct indexers). I feel that this is about as 
"automatic" as it could get. What do you think? Do you have a better idea?

Original comment by samuel.a...@gmail.com on 16 Mar 2014 at 2:05

GoogleCodeExporter commented 9 years ago 
Hello Sam,

actually I'm confused of this micro-optimisation. I don't see this API that 
much important as you do. When I started with OpenCV/JavaCV, I did not know, I 
used CvMat.get(i,j) method, which is veery slow. Any of methods suggested here 
is good for me. For performance sensitive stuff I would go with C anyway. We 
still did not rewrite our alg. to C, but I expect double performance and stable 
speed from the first run (currently, our algorithm takes 3-4 seconds to 
complete, but the first run takes 10-12 sec. due to JIT taking place).

Regarding acquireIndexer/releaseIndexer, in case of copying indexer we would 
double the memory usage, even after the work is done.

And how would the Indexable interface know, which of byte/short/... Indexer to 
return? And the readIndexer should populate any type of indexer?

I will write any version you say is good for you :)

Viliam

Original comment by viliam.d...@gmail.com on 17 Mar 2014 at 6:08

GoogleCodeExporter commented 9 years ago 
Maybe we could simplify the interface even more:

public interface Indexable {
    <I extends Indexer> I indexer(boolean direct);
}

And have an abstract release() method in Indexer. However, that way, the 
Indexable must be able to subclass the Indexer, and call the constructors for 
all the concrete classes, so having wrap() factory methods becomes moot... What 
do you think?

The Indexable would return the correct subtype of Indexer based on its own 
internal information. It doesn't occur to me that a user might want to access 
floats as bytes or something. Do you see it differently?

Or course a non-direct indexer would double memory consumption, but that's just 
a workaround for crappy JIT compilers.

It's not about micro-optimization, it's about usability. With this kind of 
interface, we could easily, for example, scale all the elements of the first 
column  this way:

Mat Amat = ...
FloatIndexer A = Amat.indexer();
for (int i = 0; i < A.size[0]; i++) {
    A.put(i, 0, A.get(i, 0) * scale);
}
A.release();

We don't need to know anything about the number of columns, or the step/stride, 
and we don't need to use some awkward "getColumn()" utility method of some 
sort: We use nothing but simple Java arithmetic. People like it when they can 
do things easily. That's why a lot of people use MATLAB, for example. But 
MATLAB is slow when it comes to loops and stuff, so it's hard to generalize. 
Java, on the other hand, with a proper compiler, can be as fast as C++, but 
unlike C++, we can make it crash proof. I'm still confused myself as to why no 
one "gets it", but I guess I'll just have to keep working on it until these 
obvious (to me at least) concepts become more widely accepted...

Original comment by samuel.a...@gmail.com on 17 Mar 2014 at 12:27

GoogleCodeExporter commented 9 years ago 
I think this interface looks good too. Is this a version I should implement?

What I meant about the double memory, that if the Indexable reused the Indexer 
instance (comment 69), it will hold double memory even after the processing is 
done.

Original comment by viliam.d...@gmail.com on 18 Mar 2014 at 5:01

GoogleCodeExporter commented 9 years ago 
Sounds good! I'll be refactoring that helper file a bit with an AbstractArray 
under AbstractMat and CvArr, but you can start implementing whenever you feel 
like it, thanks!

Releasing the memory would hurt performance, but a user that doesn't care about 
performance on a poor compiler should be using the direct one anyway, so I 
think it's better not to release it. What do you think?

Original comment by samuel.a...@gmail.com on 19 Mar 2014 at 1:47

GoogleCodeExporter commented 9 years ago 
Ok, I've added such an AbstractArray class at the top of 
helper/opencv_core.java in this revision:
https://code.google.com/p/javacpp/source/detail?r=d5bd9f3afe353618a30f0fa55a38d3
e10ce5643d&repo=presets
The plan would to make that class implement Indexable. Comments?

Original comment by samuel.a...@gmail.com on 24 Mar 2014 at 1:48

GoogleCodeExporter commented 9 years ago 
BTW, I've moved the project site to GitHub:
    https://github.com/bytedeco
So let's work over there on this from now on, thanks!

Original comment by samuel.a...@gmail.com on 29 Apr 2014 at 12:51

GoogleCodeExporter commented 9 years ago 
I started to work on a project where I needed this functionality, so I've 
implemented it here:
https://github.com/bytedeco/javacpp/commit/4d8dc9d8b244b79ca3c9d9355e476f028f3f3
6f9
https://github.com/bytedeco/javacpp-presets/commit/ab8bde373733190fe204a3bb8d183
7fbb7c62b4f

Thanks for all the discussion and help with this! It was really appreciated :)

Original comment by samuel.a...@gmail.com on 19 Oct 2014 at 12:29

GoogleCodeExporter commented 9 years ago 
Great!

Sorry for committing myself and not doing what I promised, but I really lose 
track of what is the best version and did not have time. Your version looks 
good, maybe I'll review it more thoroughly.

But you forgot to fix the original problem of this bug. See comments up to #2. 
Maybe we can keep returning cached xxxBuffer instance (as of comment #3), but 
from a deprecated method, and create the createXxxBuffer() method, that will 
return fresh instance.

Original comment by viliam.d...@gmail.com on 20 Oct 2014 at 10:01

GoogleCodeExporter commented 9 years ago 
Thanks for your review!

They've been marked as deprecated in the parent `AbstractMat` class, at least 
for the benefit of the C++ `Mat` class, and a new `createBuffer()` method 
added. But I see that the annotation doesn't get inherited by `CvMat`. It's not 
a problem anymore though because all the C++ functions that used to modify 
`CvMat` are now using `Mat` instead, so the original issue with `CvMat` doesn't 
happen anymore. Does that look OK to you?

Original comment by samuel.a...@gmail.com on 25 Oct 2014 at 1:21

GoogleCodeExporter commented 9 years ago 
Hello,

I reviewed the changes, as far I can tell, the Indexer implementation looks 
correct.

I suggest to deprecate all the get() and put() methods and the reset() method 
in AbstractCvMat, as they are very slow, in favor of the Indexer API.

Secondly, I would always add comments to deprecated methods. If you have a 
codebase and upgrade some library and it deprecates something, I'd like to know 
why and what should I replace it with.

I would mark all overriden getXxxBuffer methods as deprecated (with comments), 
just for clarity.

And I would add typed createXxxBuffer methods, as the createBuffer method 
returns Buffer, which is unusable without casting. It is a question, whether it 
should fail if changing the type here, as it is rarely useful to see, say, a 
mat with CV_16S as ints, I think...

Original comment by viliam.d...@gmail.com on 3 Nov 2014 at 7:05

GoogleCodeExporter commented 9 years ago 
And I would add comment to the createIndexer method, at least to its "direct" 
parameter. Something like:

@param direct If false, the native buffer is first copied to an Java array and 
copied back during the call to #release(). This is an optimization for 
platforms, where direct Buffer access is slow (specifically Android, but do 
your own test). Changes the behavior of the indexer in a way, that until the 
release() call, changes are not visible to the underlying object. Doubles 
memory usage.

Original comment by viliam.d...@gmail.com on 3 Nov 2014 at 7:48

GoogleCodeExporter commented 9 years ago 
I found another issue. Indexer has the release() method. The name invokes, that 
the associated resources are released upon this call. It only has associated 
resources in case of non-direct indexer, but does not release the copied array.

I can think of few solutions:

1. rename release() to finish() or commit() or copyBack(). The createIndexer 
method documentation should tell, that you should call that method after work 
in case of non-direct indexer.

2. release should release internal reference to array (or buffer) and further 
work with indexer will fail with NullPointerException (or better 
IllegalStateException)

3. The copy/copyBack logic could be encapsulated in the XxxArrayIndexer: it 
could have Pointer constructor.

I vote for 2: that is the easiest to document and use. And if the user will 
always call release (whether or not it is needed), he can safely change from 
direct to non-direct.

Another point:
The createIndexer method could be member of the Pointer class (or its 
subclasses). That way javacv needs not to have any change. And it will better 
reflect the OpenCV data structure, with the user knowing that Indexer is a way 
to access ND arrays referenced by pointers, and not a special feature of Mat. 
He could write:

mat.data_i().position(0).limit(mat.size()).createIndexer();

Original comment by viliam.d...@gmail.com on 3 Nov 2014 at 8:42

GoogleCodeExporter commented 9 years ago 
Thanks for the review I'll add a couple of `@see createBuffer()` tags for the 
deprecated methods. `createBuffer()` doesn't return `Buffer`, it returns `B` a 
subclass of `Buffer`, so I think that's OK?

As for CvMat, there are some methods that don't map to anything right now 
because I'm not sure how those should be done, so deprecating either all of 
them or just some of them doesn't make sense to me. Does it make sense to you 
in some way to deprecated functionality that has no replacement? Anyway, the 
whole class is deprecated by OpenCV, so new code should not be using that. :)

And the problem with the `direct` parameter and the `release()` is that they 
depend on the implementation. It's up to the implementing class to do whatever 
works with their underlying data. After all, if the data is available 
originally in an array, `direct` could mean a few different things, and 
`release()` be not implemented, so all these implementation details can't be 
part of the interface...

But, yes, I think it would make sense to make `Pointer` and its subclasses 
implement all that, and then it would make sense to specify the implementation 
details there. (Although they should be clear just by looking at the code :) 
Let me think about that

Original comment by samuel.a...@gmail.com on 3 Nov 2014 at 9:00

GoogleCodeExporter commented 9 years ago 
`Pointer` doesn't contain any information about the dimensions. Where would 
that come from?

Original comment by samuel.a...@gmail.com on 8 Nov 2014 at 4:29

GoogleCodeExporter commented 9 years ago 
In the end I've added `Indexer.create(Pointer ...)` factory methods:
https://github.com/bytedeco/javacpp/commit/cd632cfb0421639355a9622ce1285469dd5ff
add
It looks good like that I think. Thanks for pointing this out! This way we have 
a consistent hierarchical-like relationship between Pointer < Indexer < Mat.

Original comment by samuel.a...@gmail.com on 9 Nov 2014 at 1:05

GoogleCodeExporter commented 9 years ago 
This functionality has finally been included in JavaCPP 0.10! 
http://bytedeco.org/news/2014/12/23/third-release/
Thanks for all the help with this, and if you have any improvements, send a 
pull request! Thanks

Original comment by samuel.a...@gmail.com on 25 Dec 2014 at 11:34