Maybe a silly question but, can't we flush instead to also split the command buffer. Or does the time-out error affects all command buffers in queue?

Would using a struct instead fix the issue of the reference syntax ?

struct CryptoMatteSamples { vec2 samples[CRYPTOMATTE_LEVELS_MAX] };

Readability.

I think it would be better to just add them to the shader interface instead. I can accept this workaround for now but it should be added as a TODO that this is a temporary workaround.

Why do you need to rescan if source was already scanned by GPUSource constructor?

Even if using a constexpr uint3? This feels like a really dirty workaround and it wont work for gl_WorkGroupSize[0].

It does raise the question as to why not just add sampler_argument_buffer_bind_index_comp_? I don't think saving space inside MTLShaderInterface is critical.

Certainly not a silly question, flushing would be ideal and is what I had originally tried and hoped would resolve the problem. Theoretically, this issue should not exist in this form as compute kernels have a very very long time out.

The caveat however comes from subsequent rendering work, which is dependent on the results of compute. The issue with how the dependency tracking works here is that certain parts of the render workload can begin, as they are not dependent on the compute work, but rendering of the results of the compositor is dependent.

As such, it is the stalled graphics work which is timing out (as graphics has quite a short timeout to promote app responsiveness). The problem with flushing the command buffer is that it still allows subsequent work to become queued up behind, and when the time delta of the CPU submission to GPU execution gets too large, the command buffer can be aborted. Especially if part of its work has entered the GPU job queue.

Stalling the pipe is certainly not ideal, however it may take some re-architecting to workaround this issue, if compositing execution continues to cause these timeouts.

Cycles live viewport rendering avoids this as the Cycles compute work is decoupled into a separate command queue and texture updates occur indirectly as results become available.

A similar approach could work for the compositor in future, though if kernels are optimized and performance is reasonable, then this is also a non issue. I've only been able to reproduce this with the blur kernel, due to the nested loop and significant number of texture samples causing ~0.5 FPS on the M1 Pro GPU.

The specific cause of the stall in this case also relates to specified Metal events which chain command buffers submitted by the Metal backend, to ensure execution of command buffers happens in order.

(See encodeWaitForEvent and encodeSignalEvent in mtl_command_buffer.mm and mtl_context.mm)

Without these, while the GPU will not stall, the viewport will flicker due to possible out-of-order resource updates. I.e. a pass may press on with the GPU work, but use a non-updated version of a texture. Dependency tracking between passes will only automatically synchronise a dependency if the update is already in flight.

Apologies for the long ramble, brain dump from me as well, but just wanting to shed some light on the execution model within the backend.

One other possible solution is to selectively disable the synchronisation between subsequent command buffers, such that the compute work executes in isolation, and the viewport will then display the old results if new ones are not yet ready. Though this will likely introduce flickering, and could also cause problems with the workload submission pipe if compositor work is decoupled from the render.

In this case, it's also worth mentioning that if the GPU_finish call was isolated to known expensive kernels, then stalling the CPU does not necessarily degrade performance all that much, as the system is already heavily GPU bound in these scenarios anyway.

yes, using struct syntax would allow the conventional out type name syntax replacement to work. Pass by reference detection and replacement for array types is a little more expensive and increases the cost of GLSL -> MSL translation.

I can potentially add this, though the existing replacement is able to just perform character replacement, which is cheap. Given the syntax requirements for pass by reference of arrays, it may be a little more complex.

replace_outvars in mtl_shader_generator handles this and https://developer.blender.org/D17033 optimizes a number of these functions, so the overhead may not be as significant with the optimizations to the original replacement function.

Could possibly get the replacement code to replace the following:

out vec2 samples[CRYPTOMATTE_LEVELS_MAX]

with:

#define OUT(type, name, array) thread type(&name)[array]
OUT(vec2,samples,CRYPTOMATTE_LEVELS_MAX)

or

THD(vec2,samples, CRYPTOMATTE_LEVELS_MAX)

That should still allow for character replacement (without resizing the string), and should be fairly straightforward to implement. So long as there is no risk of Macro reuse.

Sounds good, will add the TODO for now, and look into moving these.

Good point, may have had this in for mistaken legacy reasons during port to GPUShaderCreateInfo. Recall a few cases where a shader would use a flag, but the bits were not set. Though this may have been specific to gl_FragDepth, and previously missing depth_write declarations. Will remove.

Can experiment with constexpr, there were some frustrating limitations and errors from the compiler which made this awkward to work with. I.e. things that should be considered static at compile time not being treated as such.

Though will look for a more suitable implementation.

Yeah, likely best to just use an extra parameter. The original thinking was to avoid diverging the functions which fetch the offset, but likely introducing more confusion than solving.