Regarding splitting function I talked about in description, image_scale_factor is used as correction for translation, but I think that translate x/y could be represented as vector that can be just multiplied by provided scale factor matrix. question is how much readable would that function be then.

const float translate_x = transform->xofs * data->image_scale_factor + scale_to_fit_offs_x;
const float translate_y = transform->yofs * data->image_scale_factor + scale_to_fit_offs_y;

Also there is scale_to_fit_offs_x which is not scale to fit offset actually :( It works as it should, the name is not accurate though.

Updated patch after I have found another issue. These issues are quite time consuming to test manually and hard to catch. It would be best to update current transform tests with new transform features and use proxies as well, but proxies can't be used for rendering.

Forgot to submit comment, that should explain input_has_unity_scaling_factor()

Not sure if I am explaining it correctly. But technically this preprocessing is doing 2 scaling steps at once - scaling large images to proxy size if it is used and it applies strip transform properties.

What I wrote in description about text effect seems to apply to all effects. Here I forgot to check for text effect only but that would be incorrect solution.

It is always a good idea to put explanation to the code. Makes explanation more local (so you can clearly see where it is applied to), It also makes explanation more "permanent": after a while it will be hard to track commit message down, but comment always stays in the code.

Effects operate in preview resolution, so the input image is already in correct scale. But preprocessing assumes that any image that isn't proxy needs to be scaled to match preview scale. In other words to have same scale as proxies used.

If the effect is in preview resolution, then the scaling to preview will use scale of 1, which then can be easily early-returned, without actual computation. Sounds more generic without requiring all those tricky exception cases. At least from the looks of it.

Not sure if I am explaining it correctly. But technically this preprocessing is doing 2 scaling steps at once - scaling large images to proxy size if it is used and it applies strip transform properties.

This is fine, but you can still disambiguate naming of the function. Scaling factor is rather ambiguous, while seq_is_rendered_in_preview_resolution() or seq_need_scale_to_render_size() has more clear semantic meaning.

In D10183#255649, @Sergey Sharybin (sergey) wrote:

What I wrote in description about text effect seems to apply to all effects. Here I forgot to check for text effect only but that would be incorrect solution.

It is always a good idea to put explanation to the code. Makes explanation more local (so you can clearly see where it is applied to), It also makes explanation more "permanent": after a while it will be hard to track commit message down, but comment always stays in the code.

Right I will try to be more verbose, just sometimes I look at code and think that it is pretty obvious.
If I am missing something or something needs clarification please remind me.

Effects operate in preview resolution, so the input image is already in correct scale. But preprocessing assumes that any image that isn't proxy needs to be scaled to match preview scale. In other words to have same scale as proxies used.

If the effect is in preview resolution, then the scaling to preview will use scale of 1, which then can be easily early-returned, without actual computation. Sounds more generic without requiring all those tricky exception cases. At least from the looks of it.

It depends what you mean by early-out. in this case seq_need_scale_to_render_size() will define if "early out" is possible.
You can't define it design-wise currently, because effects can use preprocessing with all features, such as strip transform, that will do scaling to preview size and image loc rot scale at once to improve performance.
Ideally effects would use almost equivalent function as input_preprocess but clearly designed to do post-processing. That would simplify code a bit, but I probably wouldn't want to do it as bugfix.

Thanks for the explanation.

From the code, there is easy-to-address feedback. Is there anything else you wanted to be checked, or you think all the cases are covered now?

In D10183#256106, @Sergey Sharybin (sergey) wrote:

Think from the code side it's fine now.

Not sure if there is something else you wanted to check (from some of the comment in the discussion I've got an impression you wanted to check something, but then I didn't see follow-up on that).

Do you think this is something we can cover with regression test?

Don't think there is anything left to check.
Tests would be probably useful, but this would have to be done in special way to use context with specific render size and image would have to be dumped then outside of render procedure, because that would force non-proxy context. So C based test probably. Haven't checked limitations there so far.