Skip to content

Conversation

@lgritz
Copy link
Collaborator

@lgritz lgritz commented Apr 14, 2025

  • image_span<T,Rank> is a 2D-to-4D bounded span with byte-measured strides (describing data layout with dimensions for channel, x, y, z) for us to have future APIs deal with bounded+strided regions rather than YOLOing raw pointers. Among other advantages, image_span will have robust bounds checking in debug builds. It also simplifies interfaces to pass a single image_span that encapsulates everything about an up-to-4D data layout, rather than needing to pass a long list of individual pointers, sizes, and strides.

  • I've templated image_span on Rank, but default to Rank=4 and am using that version only in practice. The templating by rank allows future expansion to describe one scanline (rank 2) or 2D image plane (Rank 3) if we want to do so for certain API calls, but I'm not sure yet whether to bother with that, so for now, the ability to template by Rank is just flexibility for the future.

  • Add image_span based versions of copy_image(), contiguize(), convert_pixel_values(), convert_image(), and parallel_convert_image() utilities. I added tests and benchmarks to verify their correctness and that they have similar performance to the pointer based versions.

  • Deprecate image_view, which we did not ever use internally to OIIO or in its APIs. But since we published the headers, there's a chance it's used downstream and would be unwise to change its behavior or data layout. The new image_span is what we will use moving forward so that we are free to modify it and start with a fresh slate. Keeping the definition/header for now, but marking it with a deprecation warning.

@lgritz lgritz added core APIs Affecting public APIs of core functionality classes, such as ImageInput, ImageOutput, ImageBuf. roadmap This is a priority item on the roadmap for the next major release. strategic Strategic initative with far-reaching effects (in time and code space) labels Apr 14, 2025
@lgritz
Copy link
Collaborator Author

lgritz commented Apr 15, 2025

Related design question:

By adding new span- and image_span-based equivalents for many utility functions we have that currently take raw pointers and lengths, we have the opportunity to change some design decisions because it's not a backward compatibility break.

In particular, we regrettably have some functions that look like do_thing(srcptr, dstptr) and others that look like do_thing(dstptr, srcptr). I can think of three approaches:

  1. Make all new "copy-like" functions be (dst, src) order. Arguments for: (a) I think in general we have favored this, though not been at all consistent about it; (b) same order as classic memcpy / strcpy; (c) same order as assignment dst = src.
  2. Make all new "copy-like" functions be (src, dst). Arguments for: I think a lot of people find it more intuitive to list src first, perhaps because "move this thing from here to there" is more fluent in English than "move this to here from there"?
  3. Always keep the order the same ordering as the pointer-based function we are replacing. Some will be src,dst and some will be dst,src, but global inconsistency is less important than consistency with the (eventually to be deprecated) old function that performed the analogous operation.

Thoughts?

lgritz added 3 commits April 20, 2025 15:07
* `image_span<T,Rank>` is a 2D-to-4D bounded span with byte-measured
  strides (describing data layout with dimensions for channel, x, y,
  z) for us to have future APIs deal with bounded+strided regions
  rather than YOLOing raw pointers.  Among other advantages,
  image_span will have robust bounds checking in debug builds. It also
  simplifies interfaces to pass a single image_span that encapsulates
  everything about an up-to-4D data layout, rather than needing to
  pass a function a long list of individual pointers, sizes, and
  strides.

* I've templated image_span on Rank, but default to Rank=4 and am
  using that version. The templating allows future expansion to
  describe one scanline (2D) or image plane (3D) if we want to do so
  for certain API calls, but I'm not sure yet whether to bother with
  that, so for now, the ability to template by Rank is just
  flexibility for the future.

* Add image_span based versions of copy_image() and contiguize()
  utilities. I added tests and benchmarks to verify their correctness
  and that they have similar performance to the pointer based versions.

* Deprecate image_view, which we did not ever use internally to OIIO
  or in its APIs. But since we published the headers, there's a chance
  it's used downstream and would be unwise to change its behavior or
  data layout. The new image_span is what we will use moving forward
  so that we are free to modify it and start with a fresh
  slate. Keeping the definition/header for now, but marking it with a
  deprecation warning.

Signed-off-by: Larry Gritz <[email protected]>
api(imageio.h): Add span/image_span functions

* span convert_pixel_values
* image_span convert_image and parallel_convert_image

Signed-off-by: Larry Gritz <[email protected]>
@lgritz lgritz force-pushed the lg-image_span-intro branch from 1faa4cf to 9f668c7 Compare April 21, 2025 04:11
@lgritz
Copy link
Collaborator Author

lgritz commented Apr 21, 2025

Any comments on this? If anybody has objections or comments on the basic design of image_span, please do give feedback.

In this PR, I'm just proposing the image_span itself and using it in a few trivial ways with some utility functions that previously took raw pointers and dimensions.

The next step, which I have ready to follow, is adding image_span based versions of core ImageOutput methods, which is really what's going to affect user APIs.

@lgritz
Copy link
Collaborator Author

lgritz commented Apr 26, 2025

This has been lingering for 2 weeks, plus mention at 1 or maybe 2 TSC meetings. In the absence of any objections, I will merge this by the end of the weekend and move on to the next piece of this puzzle that is queued up.

Copy link
Contributor

@EmilDohne EmilDohne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Havent had time to look into it deeply as im out on holiday this month but I like this and think its the right direction!

/// Note that the optional template parameter `Rank` includes the channels as
/// the first dimension. When no Rank template parameter is provided, it
/// defaults to 4, meaning it's an image span that can describe any choice of
/// a scanline, 2D image, or volume.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] associate these woth their respective tparam values 2, 3, 4 just to make the concept easier to understand

lgritz added 2 commits April 27, 2025 10:07
Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
@lgritz lgritz merged commit b8ffc12 into AcademySoftwareFoundation:main Apr 28, 2025
28 checks passed
@lgritz lgritz deleted the lg-image_span-intro branch April 28, 2025 07:27
scott-wilson pushed a commit to scott-wilson/OpenImageIO that referenced this pull request May 17, 2025
…4703)

* `image_span<T,Rank>` is a 2D-to-4D bounded span with byte-measured
strides (describing data layout with dimensions for channel, x, y, z)
for us to have future APIs deal with bounded+strided regions rather than
YOLOing raw pointers. Among other advantages, image_span will have
robust bounds checking in debug builds. It also simplifies interfaces to
pass a single image_span that encapsulates everything about an up-to-4D
data layout, rather than needing to pass a long list of individual
pointers, sizes, and strides.

* I've templated image_span on Rank, but default to Rank=4 and am using
that version only in practice. The templating by rank allows future
expansion to describe one scanline (rank 2) or 2D image plane (Rank 3)
if we want to do so for certain API calls, but I'm not sure yet whether
to bother with that, so for now, the ability to template by Rank is just
flexibility for the future.

* Add image_span based versions of copy_image(), contiguize(),
convert_pixel_values(), convert_image(), and parallel_convert_image()
utilities. I added tests and benchmarks to verify their correctness and
that they have similar performance to the pointer based versions.

* Deprecate image_view, which we did not ever use internally to OIIO or
in its APIs. But since we published the headers, there's a chance it's
used downstream and would be unwise to change its behavior or data
layout. The new image_span is what we will use moving forward so that we
are free to modify it and start with a fresh slate. Keeping the
definition/header for now, but marking it with a deprecation warning.

---------

Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Scott Wilson <[email protected]>
scott-wilson pushed a commit to scott-wilson/OpenImageIO that referenced this pull request May 18, 2025
…4703)

* `image_span<T,Rank>` is a 2D-to-4D bounded span with byte-measured
strides (describing data layout with dimensions for channel, x, y, z)
for us to have future APIs deal with bounded+strided regions rather than
YOLOing raw pointers. Among other advantages, image_span will have
robust bounds checking in debug builds. It also simplifies interfaces to
pass a single image_span that encapsulates everything about an up-to-4D
data layout, rather than needing to pass a long list of individual
pointers, sizes, and strides.

* I've templated image_span on Rank, but default to Rank=4 and am using
that version only in practice. The templating by rank allows future
expansion to describe one scanline (rank 2) or 2D image plane (Rank 3)
if we want to do so for certain API calls, but I'm not sure yet whether
to bother with that, so for now, the ability to template by Rank is just
flexibility for the future.

* Add image_span based versions of copy_image(), contiguize(),
convert_pixel_values(), convert_image(), and parallel_convert_image()
utilities. I added tests and benchmarks to verify their correctness and
that they have similar performance to the pointer based versions.

* Deprecate image_view, which we did not ever use internally to OIIO or
in its APIs. But since we published the headers, there's a chance it's
used downstream and would be unwise to change its behavior or data
layout. The new image_span is what we will use moving forward so that we
are free to modify it and start with a fresh slate. Keeping the
definition/header for now, but marking it with a deprecation warning.

---------

Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Scott Wilson <[email protected]>
zachlewis pushed a commit to zachlewis/OpenImageIO that referenced this pull request Sep 1, 2025
…4703)

* `image_span<T,Rank>` is a 2D-to-4D bounded span with byte-measured
strides (describing data layout with dimensions for channel, x, y, z)
for us to have future APIs deal with bounded+strided regions rather than
YOLOing raw pointers. Among other advantages, image_span will have
robust bounds checking in debug builds. It also simplifies interfaces to
pass a single image_span that encapsulates everything about an up-to-4D
data layout, rather than needing to pass a long list of individual
pointers, sizes, and strides.

* I've templated image_span on Rank, but default to Rank=4 and am using
that version only in practice. The templating by rank allows future
expansion to describe one scanline (rank 2) or 2D image plane (Rank 3)
if we want to do so for certain API calls, but I'm not sure yet whether
to bother with that, so for now, the ability to template by Rank is just
flexibility for the future.

* Add image_span based versions of copy_image(), contiguize(),
convert_pixel_values(), convert_image(), and parallel_convert_image()
utilities. I added tests and benchmarks to verify their correctness and
that they have similar performance to the pointer based versions.

* Deprecate image_view, which we did not ever use internally to OIIO or
in its APIs. But since we published the headers, there's a chance it's
used downstream and would be unwise to change its behavior or data
layout. The new image_span is what we will use moving forward so that we
are free to modify it and start with a fresh slate. Keeping the
definition/header for now, but marking it with a deprecation warning.

---------

Signed-off-by: Larry Gritz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core APIs Affecting public APIs of core functionality classes, such as ImageInput, ImageOutput, ImageBuf. roadmap This is a priority item on the roadmap for the next major release. strategic Strategic initative with far-reaching effects (in time and code space)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants