New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: add CompressionLevel to storage node, use in NRRDWriter #1035

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ihnorton
Copy link
Member

ihnorton commented Oct 29, 2018

  • add CompressionLevel setting for storage nodes
  • set default CompressionLevel=1 in vtkTeemNRRDWriter to improve writing performance

ref: https://discourse.slicer.org/t/how-to-unset-default-compress-option-during-save/4488/18

@ihnorton ihnorton force-pushed the ihnorton:comprsn_faster branch from 45cf002 to 778e1ed Oct 29, 2018

@ihnorton

This comment has been minimized.

Copy link
Member Author

ihnorton commented Oct 29, 2018

Numbers for an HCP brain MRI dataset:

  • level 9, 96s -> 495MB
  • level 1, 53s -> 505MB
  • level 0, 20s -> 2GB
  • compression off, 4s -> 2GB

(that last point is interesting, makes me wonder if the NRRD zlib interface is less efficient than it could be - level 0 should be effectively a memcopy, but it is much slower than compression off. The large improvement numbers I mentioned on discourse was tested using pynrrd rather than slicer)

@lassoan
Copy link
Contributor

lassoan left a comment

Thank you, this will be great! Almost 2x faster compression at the cost of few percent loss in compression ratio is a good deal.

Show resolved Hide resolved Libs/vtkTeem/vtkTeemNRRDWriter.h Outdated
Show resolved Hide resolved Libs/vtkTeem/vtkTeemNRRDWriter.h Outdated
Show resolved Hide resolved Libs/MRML/Core/vtkMRMLStorageNode.cxx Outdated
@ihnorton

This comment has been minimized.

Copy link
Member Author

ihnorton commented Oct 30, 2018

Almost 2x faster compression

I was hoping for quite a bit more actually, more based on experience with pure python/zlib (using pynrrd):

In [8]: %timeit -n 1 nrrd.write("/tmp/c2/dwi_9.nrrd", img, header=hdr, compression_level=9)
1 loop, best of 3: 53.8 s per loop

In [15]: %timeit -r1 -n1 nrrd.write("/tmp/c2/dwi_1.nrrd", img, header=hdr, compression_level=1)
1 loop, best of 1: 5.84 s per loop

In [16]: %timeit -r1 -n1 nrrd.write("/tmp/c2/dwi_0.nrrd", img, header=hdr, compression_level=0)
1 loop, best of 1: 701 ms per loop
@ihnorton

This comment has been minimized.

Copy link
Member Author

ihnorton commented Oct 30, 2018

Actually, my current Slicer is built in debug mode, so might see better speed-up in a release build.

@pieper

This comment has been minimized.

Copy link
Member

pieper commented Oct 31, 2018

Looks good to me (didn't test). Let us know how release mode goes.

This might avoid the need to expose compression options in the save dialog gui.

Murat and I have been discussing bundling pigz with Slicer, but it needs to be cmakeified and could have a number of lingering issues (not sure about its status on windows). So I hope this compression level change satisfies the 80% case.

@ihnorton

This comment has been minimized.

Copy link
Member Author

ihnorton commented Oct 31, 2018

I'm not planning to do a release build locally right now (building deps takes hours). I think even ~2x boost is enough to justify this, though, because it doesn't add much complication...

@pieper

This comment has been minimized.

Copy link
Member

pieper commented Oct 31, 2018

Now that we are in post-release 'break things' mode I have no objections to merging this so Murat and others can test. I don't see much risk for downsides.

@lassoan

This comment has been minimized.

Copy link
Contributor

lassoan commented Oct 31, 2018

@Sunderlandkyl works on compressed video storage (mkv file reading/writing/replaying works very nicely by the way). He will add CompressionParameter attribute to vtkMRMLStorageNode. It'll be a string parameter that can be populated from presets provided by concrete storage nodes. This can handle complex presets, such as video compression settings, and can be utilized for simple zlib compression settings (low/medium/high).

Until we receive that pull request, I think we should merge this as is, so that we can test different compression levels. Kyle will update CompressionParameter storage to the new API in his pull request.

@pieper

This comment has been minimized.

Copy link
Member

pieper commented Oct 31, 2018

Have you thought about exposing this in the Save dialog too? Would mean changing the checkbox to a combobox.

@Sunderlandkyl

This comment has been minimized.

Copy link

Sunderlandkyl commented Oct 31, 2018

The plan that I discussed with @lassoan was that the save dialog would display an additional column containing a combobox if UseCompression was checked.

It could also be implemented as a single combobox, but then there is a potential disconnect with the two parameters (UseCompression + CompressionParameter) being represented with a single combobox.

I could see it being done either way.

@pieper

This comment has been minimized.

Copy link
Member

pieper commented Oct 31, 2018

It would be nice if the GUI just follows what's in the node. If we're keeping the compression boolean and the level then yes, we can show both in the GUI.

@ihnorton

This comment has been minimized.

Copy link
Member Author

ihnorton commented Oct 31, 2018

Thanks for the reviews. Committed in r27529 / d87fdfd

@ihnorton ihnorton closed this Oct 31, 2018

@ihnorton ihnorton deleted the ihnorton:comprsn_faster branch Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment