Skip to content
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

Add a simple CSV-based image format to halide_image_io #8169

Closed
wants to merge 6 commits into from

Conversation

steven-johnson
Copy link
Contributor

@steven-johnson steven-johnson commented Apr 2, 2024

This is intended to be used with RunGen to facilitate passing small image data on the commandline when desirable or necessary. It pains me to create a bespoke image format here, but there doesn't appear to be a well-known image format that fills this need:

  • plaintext
  • capable of representing ~all the numeric types that Halide can handle (though bf16 is TODO in this PR)
  • capable of representing arbitrary dimensionality
  • doesn't require a complex library dependency to read (e.g. libTIFF)

I plan on making use of this format in some upcoming improvements to RunGen and the Visualizer code.

This is intended to be used with RunGen to facilitate passing small image data on the commandline when desirable or necessary. It pains me to create a bespoke image format here, but there doesn't appear to be a well-known image format that fills this need:
- plaintext
- capable of representing ~all the numeric types that Halide can handle (though f16/bf16 are TODO in this PR)
- capable of representing arbitrary dimensionality
- doesn't require a complex library dependency to read (e.g. libTIFF)

I plan on making use of this format in some upcoming improvements to RunGen and the Visualizer code.
@steven-johnson steven-johnson requested a review from abadams April 2, 2024 16:31
@zvookin
Copy link
Member

zvookin commented Apr 2, 2024

Description indicates a data format is being created. I do not see adequate documentation provided. On line 1215 of halide_image_io.h in the PR, there is a comment giving a minimal idea, but it doesn't describe how the data items are formatted and what is allowed, and it does not explain how the list of data items is flowed into the dimensions of the output.

Beyond this,

  • This needs some kind of versioning. I suppose you could stipulate that it never evolves and should be thrown away if it has to, but that seems a bit dubious.
  • Are CSV header lines supported?
  • Are multiple lines of CSV supported? What happens with them?
  • Are mins and strides worth supporting?

Is it possible to make Python a dependency and have the inputs just be python scripts that generate the image? A stylized plain text format arises naturally from that.

@steven-johnson
Copy link
Contributor Author

Rather than answering the specifics, let me give background: This grew out of frustration with wanting/needing to make our Visualizer tool easier to use (and add CMake rules to help out). Specifically, I want users to be able to add visualization by adding a single CMake rule to their build files; say we have a Generator like

add_halide_generator(camera_pipe.generator
                     SOURCES camera_pipe_generator.cpp
                     LINK_LIBRARIES Halide::Tools)

I'd like to be able to add a rule that will use RunGen to execute the Generator output, then process it into an .mp4 for visualization, e.g.

add_halide_visualization(camera_pipe_process_viz_out
                          GENERATOR camera_pipe.generator
                          GENERATOR_NAME camera_pipe
                          MP4_NAME camera_pipe_process_viz.mp4
                          TARGET_CMDLINE_ARGS
                            input=${CMAKE_CURRENT_LIST_DIR}/../images/bayer_small.png
                            matrix_3200=path/to/this/thing.tiff,
                            matrix_7000=path/to/this/thing2.tiff,
                            color_temp=3700
                            gamma=1.8
                            contrast=50
                            sharpen_strength=1
                            blackLevel=25
                            whiteLevel=1023
                            processed=out.png)

So far, this looks good; the problem is that the only way to get Buffer inputs into RunGen at present is via a file, and this becomes obnoxious to do for small inputs and especially for float-buffer inputs, since TIFF is the only format we support that supports float... but we don't support loading TIFF, only saving it, because loading TIFF is too complex to do without library support. (And even if we decide to add libTIFF as an optional dep, the way we do for JPG and PNG, that still makes it unpleasant to work with for small chunks of data since we'd still want tooling to convert to/from text... and would still leave us without any way to specify f16 inputs, which is a glaring hole in our support)

For this specific case, the matrix_3200 and matrix_7000 inputs are just 4x3 matrices of float32; a simple all-text format would allow us to specify them inline, in the build rule, which was the main motivating case for a format of this sort.

So, before addressing the obvious shortcomings of this thing I'm proposing, let me back up and ask for better guidance: am I missing a better option here? Are there other, better ways of plugging this hole that I'm unaware of?

steven-johnson added a commit that referenced this pull request Apr 2, 2024
(Changes extracted from #8169, which may or may not land in its current form)

Some missing support for _Float16 that will likely be handy:
- Allow _Float16 to be detected for Clang 15 (since my local XCode Clang 15 definitely supports it)
- Expr(_Float16)
- HALIDE_DECLARE_EXTERN_SIMPLE_TYPE(_Float16);
- Add _Float16 to the convert matrix in halide_image_io.h
@steven-johnson
Copy link
Contributor Author

I'm gonna close this and think some more about it.

steven-johnson added a commit that referenced this pull request Apr 4, 2024
(Changes extracted from #8169, which may or may not land in its current form)

Some missing support for _Float16 that will likely be handy:
- Allow _Float16 to be detected for Clang 15 (since my local XCode Clang 15 definitely supports it)
- Expr(_Float16)
- HALIDE_DECLARE_EXTERN_SIMPLE_TYPE(_Float16);
- Add _Float16 to the convert matrix in halide_image_io.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants