Closed calvertdw closed 9 months ago
That sounds like an issue with the API of HDF5? I mean, a different function gets called, right?
I'm having trouble tracking it down. That read call is:
public native void read(Pointer buf, @Const @ByRef DataType mem_type);
So, they are calling the same method since they both extend Pointer. I'm not sure what lives on the other side of that native call.
Oh , actually, there is also a
public native void read(@StdString @ByRef BytePointer buf, @Const @ByRef DataType mem_type);
So that one is a different method. All other pointers would be calling the Pointer one.
Would the @StdString
cause some kind of null termination issue? Maybe we should remove that.
Ah, yeah. In hdf5's H5DataSet.cpp
, there are two functions:
void DataSet::read(void *buf, const DataType &mem_type, const Data
const DSetMemXferPropList &xfer_plist) const
and
void DataSet::read(H5std_string &strg, const DataType &mem_type, const DataSpace &mem_space,
const DataSpace &file_space, const DSetMemXferPropList &xfer_plist) const
So when we use a BytePointer, we are accidentally calling the string
ones.
Note this would also be an issue for the cooresponding write
methods, which somehow work, but we should still fix it,
Btw, until this gets fixed, for writing/reading all you have to do is cast to Pointer to get it to call the correct method, like this:
dataSet.read((Pointer) dataPointer, dataType);
No need to muck around with ShortPointer or anything.
Yes, that's how we need to do it. What kind of fix do you have in mind?
So I was thinking of re-mapping the read(H5std_string &strg
and write(H5std_string &strg
C++ signatures to readString(String
and writeString(String
on the Java side. Is there a way to do that? (Java side readString(BytePointer
would be just fine as well)
Yeah, it should be possible see https://github.com/bytedeco/javacpp/wiki/Mapping-Recipes#specifying-names-to-use-in-java, but that's a problem with the C++ API. If you call read()
or write()
with a char*
in C++, you're probably going to end up calling the "wrong" function as well.
In my opinion, that's something you should be reporting upstream: https://www.hdfgroup.org/community/community-development/
Good thing we have a good workaround, I could see that one taking a while to get changed :laughing:.
I reported a bug there: https://jira.hdfgroup.org/browse/HDFVIEW-297
@calvertdw why not create a Github issue here: https://github.com/HDFGroup/hdf5/issues ?
Great point. The other was getting no activity. Done!
We can add those as helper methods if you want. Sounds good?
Are the strings encoded according to Java's modified UTF-8 encoding? https://docs.oracle.com/javase/6/docs/api/java/io/DataInput.html#modified-utf-8
If so, isn't the correct way to read this via the JNI function NewStringUTF
?
https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#NewStringUTF
That's how hdf.hdf5lib.H5Dread_string
is implemented. So a simple implementation should just be passing the id
field of the C++ H5DataSet
to hdf.hdf5lib.H5Dread_string
, right?
https://github.com/HDFGroup/hdf5/blob/81bc34ac4c3f42532edee32095651f6bcd5e55a2/java/src/jni/h5dImp.c#L993-L1054
@mkitti You are misundertanding the issue I was having. I was trying to read raw data (encoded JPEG images) from an HDF5 file. I had a BytePointer
JavaCPP object and called the HDF5 read method. What I got was only a handful of bytes until the first '0' byte occurred. This was a major head scratcher and I had to dig in to find out I was accidentally calling the String function.
The problem is that to read/write literally just raw data, HDF5 is making that risky by inviting the opportunity to accidentally call a version of read
that truncates the read on the first '0'. As that is the most basic of data storage functions, I am a bit dissappointed that HDF5 is not super clear about what those functions do and also super clear about the data truncation in the string case. There is not even documentation on those most basic of functions in the code, which is what I would read as a user of the C++ API.
I think https://github.com/HDFGroup/hdf5/pull/3083 still fixes the situation. Even if you read it as a std::string
by accident that fix will read the entire JPEG into the std::string
. It will not stop at the first null byte since we no longer use the C string convention.
@mkitti Sounds like a fix to me. When does it get released?
Well it got merged into the main development branch, so now it is just a matter of the HDF5 release schedule in their README. https://github.com/HDFGroup/hdf5/blob/develop/doc/img/release-schedule.png
It's still not the fix I was hoping for, because the correct way to read/write bytes is to use the standard read
and write
functions. The C++ code does seem reasonable, it seems like the issue is now that JavaCPP loses the type difference between a String
and ByteBuffer
by using BytePointer
for both. So for all C++ libraries that have signatures where the only difference is a void*
vs a std::string
would have an issue.
Even if keeping using the string version, I think users would still sometimes get errors, from the changlog of https://github.com/HDFGroup/hdf5/pull/3083:
Fixed length datasets are now read into H5std_string as a fixed length string of the appropriate size. Variable length datasets will still be truncated at the first null byte.
A JPEG image is variable length, after all.
It still sounds like an issue with HDF5 to me. The read() function has no idea how much memory was allocated in the pointer it was given, so that's a safety issue. There should be a size parameter somewhere to indicate the maximum size. That's not an issue with std::string since that can allocate as much memory as we need.
A JPEG image is variable length, after all.
How so? Its length is not determined by some token in the byte sequence.
When you write the bytes of the image, you tell HDF5 how many bytes you want to write.
The read() function has no idea how much memory was allocated in the pointer it was given, so that's a safety issue.
The memory space argument is meant to indicate how many bytes have been allocated.
See the docs for the C function H5Dread
.
https://docs.hdfgroup.org/hdf5/v1_14/group___h5_d.html#ga8287d5a7be7b8e55ffeff68f7d26811c
Ok, so inversely, why do we need to that memory space argument for std::string??
Ok, so inversely, why do we need to that memory space argument for std::string??
You do not need it in C++. It defaults to DataSpace::ALL.
That appears to get translated to this via JavaCPP:
public void read(@StdString @ByRef BytePointer buf, @Const @ByRef DataType mem_type)
However, you can specify a file dataspace if you only want to read some of the bytes and copy it into a specific location of a buffer via memory space.
BTW, does it make sense to map H5std_string
to BytePointer
at all? Or should that be reserved for char*
?
BytePointer
does not make very much sense to me a peer to std::string
.
Perhaps java.lang.StringBuffer
or StringBuilder
would be better?
There isn't anything interesting about StringBuilder
from a JNI point of view, we might as well just pass the internal char[]
in that case, which already works with std::u16string
, but I don't think HDF5 supports that. I think it only supports std::string
, which doesn't support UTF-16, so that's probably not an interesting idea either.
It seems like JavaCPP should have a new type to match with a std::string
, then? I know that'd be quite some work.
Java has String
, StringBuffer
, and StringBuilder
.
String
is immutable.StringBuffer
is mutable and synchronizedStringBuilder
is mutable and not synchronizedstd::string
is mutable and not synchronized.
It seems like JavaCPP should have a new type to match with a
std::string
, then? I know that'd be quite some work.
No, it's not difficult. I've added support for that in commit https://github.com/bytedeco/javacpp/commit/7227ec6846cfba24eb4ccf18a7945f6f8cd45ccd, so we can add something like that to HDF5's presets:
infoMap.put(new Info("std::basic_string<char>", "std::string", "H5std_string").pointerTypes("H5std_string").define());
But it's not exactly a nice (efficient) interface for a string and that replaces all instances of std::string
... Probably not what you want.
Java has
String
,StringBuffer
, andStringBuilder
.
We can access String
efficiently from JNI, not so with StringBuffer
or StringBuilder
.
That looks great! That sounds like it will absolutely solve this issue, since this is about saving raw data with HDF5 and avoiding accidental calls to the string
functions.
Looking at this again, it seems like everything works like we want when casting explicitly to Pointer
, and if we don't cast, it just goes through a std::string
, which might be less efficient, but it still works correctly with the latest fixes in HDF5, so I don't see any problems the way things are now. Can we close this? @calvertdw
Yes. Thank you!!
When 0s are in the data, I find I must use a ShortPointer to get the full data for some reason. I'm not sure why. The top test case passes, native bytes and be written and read correctly, the second test fails.