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

allow varying size inputs, with varying size channels #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fdan
Copy link

@fdan fdan commented Jul 1, 2022

This change addresses a todo item outlined in MLClient.cpp:

// Parse image. TODO: Check for multiple inputs, different channel size

The previous code enforced a single format on all inputs. I needed to make this change in my own fork and thought I'd submit it to Foundry for comment.

Copy link
Contributor

@ringdk ringdk left a comment

Choose a reason for hiding this comment

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

Hello! Thanks for submitting the merge request. I can see what you're looking to achieve, however there are a couple of things that need to be addressed:

  • The reason for using the format() and not the inputIop's bounds is because the format gives us the full size frame, and not just a data window. As you've found, it's not ideal.
  • When changing it to imageBounds you need to be careful about handling negative values, and computing the correct width & height for the buffer. (eg. on your lines 467 & 482, it is possible for the values for fr and ft to be negative)
  • Similarly, the buffers could be allocating more memory than is needed, i.e. you probably want [fr-fx-1, ft-fy-1] for the dimensions
  • The data window isn't being passed through to the server
  • The renderOutputBuffer() function is still parsing using the returned image using the format and not the imageBounds

Flagging this to @johannabar @guillaumegalesfoundry , as I'm wondering if ImagePlane can take more than 4 channels.

Thanks again @fdan !

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.

None yet

2 participants