API changes to stream2 CBOR parsing

[GDYendell]

This ticket is for the outcomes of our meeting today.

• Remove decompression from stream2_parse_msg and create a new function to perform decompression that can be optionally called afterwards
• Add compression encoding string (e.g. "bslz4") to stream2_array - or somewhere else in the cbor structs - to allow DAQ pipelines to correctly handle the compressed data
• Add stream2_parse_msg_header (better name welcome) that only populates the members of stream2_msg and then stops parsing even if there is content remaining in the cbor buffer

There a couple of other things that aren't such a problem to maintain in a fork but would be nice to resolve if it doesn't cause you any problems.

• I made the stream2 library SHARED so that I can build dynamically loadable plugins. Is there a reason this is STATIC currently, or it could it be changed?
• I removed compression from the stream2 target_link_libraries and removed the include and code that used it. This isn't a huge issue, but I did this because it wouldn't compile, I got the following error:

Build Error

``` [build] cd /workspaces/eiger-detector/vscode_build/third_party/dectris/stream_v2/examples/third_party/compression && /bin/g++ -Dcompression_EXPORTS -I/workspaces/eiger-detector/vscode_build/include -I/workspaces/eiger-detector/data/include -I/workspaces/eiger-detector/data/common/include -I/workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression -I/workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/third_party -g -fPIC -std=gnu++11 -o CMakeFiles/compression.dir/src/compression.c.o -c /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c:42:8: error: ‘_Bool’ does not name a type [build] static _Bool parse_header(const char** const src, [build] ^ [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c:56:8: error: ‘_Bool’ does not name a type [build] static _Bool decompress_bslz4_block(char** const dst, [build] ^ [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c: In function ‘size_t decompress_buffer_bslz4_hdf5(char*, size_t, const char*, size_t, size_t)’: [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c:94:61: error: ‘parse_header’ was not declared in this scope [build] if (!parse_header(&src, src_end, &orig_size, &block_size)) [build] ^ [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c:110:38: error: invalid conversion from ‘void*’ to ‘char*’ [-fpermissive] [build] char* tmp_buf = malloc(block_size); [build] ^ [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c:122:46: error: ‘decompress_bslz4_block’ was not declared in this scope [build] elem_size)) [build] ^ [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c:130:63: error: ‘decompress_bslz4_block’ was not declared in this scope [build] last_block_size, elem_size)) [build] ^ [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c: At global scope: [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c:147:8: error: ‘_Bool’ does not name a type [build] static _Bool decompress_lz4_block(char** const dst, [build] ^ [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c: In function ‘size_t decompress_buffer_lz4_hdf5(char*, size_t, const char*, size_t)’: [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c:178:61: error: ‘parse_header’ was not declared in this scope [build] if (!parse_header(&src, src_end, &orig_size, &block_size)) [build] ^ [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c:197:66: error: ‘decompress_lz4_block’ was not declared in this scope [build] if (!decompress_lz4_block(&dst, &src, src_end, block_size)) [build] ^ [build] /workspaces/eiger-detector/data/third_party/dectris/stream_v2/examples/third_party/compression/src/compression.c:201:71: error: ‘decompress_lz4_block’ was not declared in this scope [build] if (!decompress_lz4_block(&dst, &src, src_end, last_block_size)) [build] ^ [build] gmake[2]: *** [third_party/dectris/stream_v2/examples/third_party/compression/CMakeFiles/compression.dir/src/compression.c.o] Error 1 [build] gmake[1]: *** [third_party/dectris/stream_v2/examples/third_party/compression/CMakeFiles/compression.dir/all] Error 2 ```

I think it should just use bool so that it works in C++? https://stackoverflow.com/a/3530061/6694972

[kalcutter]

@GDYendell A few comments to your issues.

• The reason it's STATIC is simply to produce a single standalone example binary that doesn't depend on other build shared libraries. Another reason is that the compression dependency is already STATIC, so this was the simplest way to avoid link errors.
• The compression library is C99. It should be compiled as C. That code is built whenever the dectris-compression python library gets installed and must be extremely portable. You should build the compression library as a separate C library.

Could you maybe provide a CMakeLists.txt that expresses how you want to build your software?

[GDYendell]

OK I have it building without changing it to SHARED and including the compression lib now. See here.

I can build my dynamic library against a stream2 lib that uses STATIC as long as I add the -fPIC compiler flag for stream2 and tinycbor. See here. Would this be an acceptable change?

Adding set(CMAKE_C_STANDARD 99) to my top level CMakeLists allows it to compile compression - for some reason the same command here does not work, I guess because it is not the top level. I would rather not have to do this as it could be restrictive to the rest of the project. It also works if I add the compile flag directly in the relevant places. See here and here. Is there a better way to do this? I feel like it should be possible to define it in the stream2 CMakeLists and have that apply to everything in the tree, but it does not seem to.

Maybe set_property(TARGET tgt PROPERTY CXX_STANDARD 11) would work? https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html

[sdebionne]

I second the need for compressed data access for two reasons:

• Decompression is single threaded and happens in the context of the acquisition thread;
• Compressed data should be accessible e.g. to avoid decompressing/recompressing for no reason if no transformation are applied.

Would you accept a PR that implements this?

[kalcutter]

@sdebionne First, let me start off by saying that this code was primarily intended only as an example. At the same time, it was written with considerable care, such that it could also be used verbatim as an unofficial library. Would you see it as a benefit if this was more of an official library? Or is the status quo also acceptable assuming it includes maintenance?

In general, we are open to PRs. However, in this case we are also considering adding a C++ example that includes the feature you request. Would C++ also be acceptable, or do you think C is more appropriate? How urgent do you need this feature?

[sdebionne]

Hi @kalcutter, I understand that this code is primarily an exemple, but if it can be turned into a officially supported library (with the help of the community) that would be even better for us.

C works for me, since C++17 is our target platform we are all for C++. But I guess it might make binding to other languages mode complicated. With C++ in mind, I would like to have more control on the memory management, either using memory_resource if memory has to be managed by the library, or using OutputRange if the memory is provided by the client (probably better).

How urgent do you need this feature?

Actually I was about to start coding it... Using the already compressed buffer chunk to write HDF5 file is a recurrent feature request on our side. We would still decompress the data to compute Region-Of-Interest statistics and other "const" operations.

What would be your timeframe for this C++ example?

[kalcutter]

Hi @sdebionne, Thanks for the input. So to solve the immediate issue, I think its best to update the current C example. I will look into this today and put the changes on a branch for feedback sometime this week.

[kalcutter]

Hi @sdebionne @GDYendell, I have pushed a new branch stream2-no-decompress that removes decompression from stream2.c. Please try to integrate these changes and give any feedback ASAP.

• stream2.c no longer handles any decompression.
• This means that now only example.c depends on dectris-compression.
• There is a new type stream2_bytes that represents a contiguous byte string in either compressed or uncompressed form. The compression field may be checked to know the algorithm used. If compression.algorithm is NULL then the referenced data is uncompressed. The referenced data is represented by the ptr and len fields. ptr points directly into the CBOR buffer. This works robustly in our case because we do not use CBOR chunking.
• Since ptr points directly into the CBOR buffer, it is unaligned. This means care must be taken when accessing uncompressed data directly though that pointer to avoid undefined behavior. The benefit of this is there is no longer any copying of large data.
• All arrays now represent their data with stream2_bytes.
• Specialized array types have been removed. There is now a single type string2_typed_array to represent all typed arrays. Similarly, there is now a single type stream2_multidim_array to represent all multi-dimensional arrays. Therefore, more code is likely required to directly access the array data for fields that used to be specialized.
• Also keep in mind that flatfield/pixel mask data may be sent compressed in the future (perhaps opt-in though).

[GDYendell]

Thanks @kalcutter I will try too look at this soon.

Do you know if the frame capture data here is up to date / compatible with this latest stream.c logic?

[kalcutter]

@GDYendell That data is not compatible with the current examples. That is old incompatible data from the preview release.

[kalcutter]

Would it be helpful if I committed test data to this repository?

[GDYendell]

Would it be helpful if I committed test data to this repository?

Yeah that would be great!

[kalcutter]

I have pushed some test data to a new branch stream2-test_data. We may move it to another repository in the future since it's a bit large.

[sdebionne]

Thanks @kalcutter ! I had a quick look at the code and it sounds like it answers our needs perfectly. I'll give you more feedback once I have integrated it in our system.

[GDYendell]

Thanks @kalcutter! I think this works OK for me - even the user data :smile:.

I do still have to insert -fPIC to the stream2, cbor and bitshuffle CMakeLists to be able to compile it into a dynamic library (e.g.). Is there a way around this or will I have to maintain that in a fork?

A few comments / questions:

• These fields are missing - is this expected?
  • detector_serial_number
  • images_per_trigger
  • number_of_triggers
  • roi_mode - just because this is from a 500K?
  • goniometer - does this not appear if the config is not set on the detector?
• Is data channel 3 - which I believe is the difference between channel 1 and 2 - necessary? We could calculate this client side if we need it without using up network bandwidth off of the back of the detector. Can it be disabled?
• On the same note, can data for threshold 2 be disabled if it is not needed?

[kalcutter]

I will look into your -fPIC issue. Can you make a separate issue for that?

All documented fields are implemented and will be available on a running system. detector_serial_number and goniometer are documented and only missing from the test data. goniometer, specifically, is only present if goniometer parameters are configured, in my case they were not.

The other missing fields: images_per_trigger, number_of_triggers and roi_mode are currently not supported. We intentionally did not include every possible field from the SIMPLON API. Generally, we would like to avoid fields that are merely artifacts of the API and do not directly describe the data. However, additional fields may be added in the future if they make sense, pending feedback. Please open a separate issue for each unsupported field that you require explaining the use-case and we will consider it. As a workaround, you can always put any missing fields in user_data, however, the C example currently doesn't doesn't expose user_data. Please open an issue if you want to see user_data in the C API.

I included the threshold_2 and difference channels in the test data so you could know what to expect when they are enabled. Enabling these channels is optional. If they are not enabled, their data will not be present.

[GDYendell]

Added #3 for building shared libs.

... are currently not supported.

OK good to know. I will ask if we have a use case for those and take them out for now.

If they are not enabled, their data will not be present.

OK that's great. It is useful to have the test data with it included, as you say.

Also, I think it probably would be best to keep the test data separate from this repo as the size not ideal when cloning this repo as a submodule. Having it available and versioned against this library would be very useful. Although presumably it won't change much going forward.

[kalcutter]

Also, I think it probably would be best to keep the test data separate from this repo as the size not ideal when cloning this repo as a submodule. Having it available and versioned against this library would be very useful. Although presumably it won't change much going forward.

Yes I agree. I only put them here temporarily for convenience.

[kalcutter]

@sdebionne We would love your feedback too. :smiley:

[sdebionne]

I am on it 😁 !

[sdebionne]

Hi @kalcutter, sorry for the delay, I have just finished moving my code to your branch and I am happy with the changes! The only changes I would suggest are in the client code, so no big deal. Thank you again, I really appreciate your reactivity.

About the test data, I would also like to have in a different repo (this one would need to be cleaned with a tool like BFG Repo-Cleaner).

[kalcutter]

@sdebionne Great! Please give me your suggestions! What do you mean "client" code? example.c? client.py?

I have removed the test data branch. The data will no longer be fetched when checking out the repo. Why do you think additional cleaning is necessary?

[sdebionne]

By client code, I meant the example that consumes the "Stream2 C API".

I have removed the test data branch. The data will no longer be fetched when checking out the repo. Why do you think additional cleaning is necessary?

I would think that git removes not the commits but the branch ref only. Maybe they are orphan and cleanup after a while though.

[kalcutter]

By client code, I meant the example that consumes the "Stream2 C API".

@sdebionne Please send any suggestions. :-)

[kalcutter]

@sdebionne @GDYendell If the API is OK for both of you, I will apply the changes to the main branch. Any objections?

[kalcutter]

@sdebionne @GDYendell I have rebased these changes and applied them to main. Please reference the commit on main. I will delete the old branch next week.

[kalcutter]

@GDYendell Regarding stream2_parse_msg_header: I gave this some thought and I don't think it makes sense to implement in this repository. Is there any reason you can't just use tinycbor to parse the two fields that interest you (and stop early to avoid errors)?

[GDYendell]

@kalcutter I have actually re-implemented some things on my side to make it much easier to work with the full message, so I am happy to drop this request.

[kalcutter]

@GDYendell Great. Are there any points still open?

[GDYendell]

@kalcutter I don't think so. We will be rolling out the latest detector software across Eigers at DLS in the coming weeks so we will see how that goes, but it is working well with the simulator.

[GDYendell]

I included the threshold_2 and difference channels in the test data so you could know what to expect when they are enabled. Enabling these channels is optional. If they are not enabled, their data will not be present.

@kalcutter is there a config in the SIMPLON API to control which channels are sent?

[kalcutter]

@GDYendell you simply need to enable the thresholds/difference mode as described in the official documentation (search the document for "threshold/n/mode"): https://media.dectris.com/210607-DECTRIS-SIMPLON-API-Manual_EIGER2-chip-based_detectros.pdf

[GDYendell]

Ah perfect, I had not seen this latest manual - I will have a read through. Thanks!