Warning being logged in Safari when using a filter shader in 2D mode

[davepagurek]

Most appropriate sub-area of p5.js?

• [ ] Accessibility
• [ ] Color
• [ ] Core/Environment/Rendering
• [ ] Data
• [ ] DOM
• [ ] Events
• [ ] Image
• [ ] IO
• [ ] Math
• [ ] Typography
• [ ] Utilities
• [X] WebGL
• [ ] Build Process
• [ ] Unit Testing
• [ ] Internalization
• [X] Friendly Errors
• [ ] Other (specify if possible)

p5.js version

1.9.0

Web browser and version

Safari 17.0

Operating System

MacOS 14

Steps to reproduce this

Steps:

1. Create a custom filter with createFilterShader
2. Create a 2D mode canvas
3. Use the filter shader on it

On safari this warning gets logged:

Weirdly, this doesn't happen in Chrome/Firefox, but honestly I'm not sure why it doesn't. It seems to be happening because when we call copy() to copy the 2D mode contents to the filter graphic, we're passing in the p5.Renderer2D as the source: https://github.com/processing/p5.js/blob/37d3324b457b177319ed65468201f2806a66eff5/src/image/pixels.js#L602

Technically we do not validate p5.Renderer as a valid input for texture. Maybe we should?

Snippet:

See the bouncing balls section of this sketch: https://openprocessing.org/sketch/2107682

[deveshidwivedi]

Hi! @davepagurek Could we use get() function instead of copy() to transfer the contents of the main canvas to the filter graphics layer?

[davepagurek]

Hi @deveshidwivedi! The reason we don't use get() here is that get() involves loading the pixels from one canvas and putting them into a p5.Image. This has a lot of overhead compared to just using the existing canvas as a texture, so we're avoiding it for performance reasons.

[deveshidwivedi]

Thanks for the clarification!

[rohanjulka19]

Hi, I looked around a bit and the issue seems to be due to how the stacktrace is generated in safari vs chrome. So whenever _validateParameter is called if the parameters are invalid it logs a message in console. But if the current function due to which invalid error was logged on console calls another function whcich inturn also calls _validateParameter then another console message will be logged which should not happen as that function is not directly being called by the user.

To mitigate this the generated stacktrace is parsed to identify whether this function has been direclty called by the user or has been invoked by another function internally. Now to identify that the code checks whether the function which invoked the function calling the _validateParameter function is in belongs to p5 or not.

https://github.com/processing/p5.js/blob/daba49806a389e98cd63c62667928f61243c7fa1/src/core/friendly_errors/validate_params.js#L616

This was done as part of Google Summer of Code by akshay-99. Below is the link of the readme file written by akshay-99 hihglighting all the work done which includes addressing this issue. The image is also picked from there only.

https://github.com/processing/p5.js/blob/main/contributor_docs/project_wrapups/akshaypadte_gsoc_2020.md#part-1-addressing-known-problems-with-the-fes

In chrome and mozilla the stack trace generated includes function names but in case of safari the stack trace does not have function names instead they are replaced with anonymous functions due to which the above check fails and warning is thrown in safari. This is happening because in p5.js functions are defined as inline functions due to which they are being treated as anonymous functions by safari.

I am not sure what's the best way to fix this but like one hacky way that seems to be working is to check the filename of the function instead and only logging to console if it is not p5 or p5Custom.

[davepagurek]

Ahh I see, so to summarize, in other browsers, _validateParameters shouldn't do anything when in a nested p5 call, only the one directly called by a user, and that's why the message was suppressed in Firefox/Chrome.

A heavier solution for this could be to stop parsing the stack trace, but instead wrap every top-level function call in a decorator like this, that counts how nested we are:

let nesting = 0
function topLevelCall(method) {
  return function(...args) {
    nesting++;
    try {
      method.apply(this, args);
    } finally {
      nesting--;
    }
  };
}

// Usage:
p5.prototype.createFilterShader = topLevelCall(function(vert, frag) {
  // ...
})

Then in FES, you can only log an error if nesting === 1.

[rohanjulka19]

Yeah that makes more sense. Maybe a validateParameter wrapper which will validate the arguments and if a warning is logged will set a global variable which will be used to decide if error should be logged or not functions and on completion of execution of the function inside the wrapper the flag can be reset.

function validateParameterWrap(method) {
    return function (...args) {
           p5.errorLogged = _validateParams(method, arguments);
           method.apply(this, args);
          p5.erroLogged = false;
    }
}

p5.prototype.createFilterShader = validateParameterWrap(function(vert, frag)

This can also be set as a decorator, based on this article a babel plugin can be used for that.

https://blog.logrocket.com/understanding-javascript-decorators/

Just brainstorming.

[davepagurek]

One small update to the code, I think it'd need to also pass the name:

p5.prototype.createFilterShader = validateParameterWrap('createFilterShader', function(vert, frag) { ... }))

I think combining the logging logic + validating parameters would make sense. I think the implementation would still need to have a nesting count in order to know whether or not the error should be logged though?

@limzykenneth you've also looked into this a bit in the past, what do you think of this approach?

[rohanjulka19]

I think combining the logging logic + validating parameters would make sense. I think the implementation would still need to have a nesting count in order to know whether or not the error should be logged though?

Aplogies for the delay in response. Yeah I guess nesting count will be required. I was considering of using something like a lock variable which can only be set and reset by the top level function after calling _validateParams, but keeping a nesting count seems like a better and a straightforward approach.

[limzykenneth]

Sorry took a bit of time to get to this. @rohanjulka19 Very good investigative work to find this and a proposed implementation.

I like this general idea with the validateParameterWrap function which can help us move towards addon/external library use of FES that is considered part of the stretch goal for 2.0.

What I'm thinking about right now is how to possibly make the footprint smaller as with the current proposed implementation, all functions will need to be wrapped by validateParameterWrap. Thinking out loud here so may not make sense, but what if we consolidate the parameter validation wrapping to a single place that iterate through say the p5 object and warp each function that way?

// Similar implementation as above
function validateParameterWrap(method) {
    return function (...args) {
          // Checks to see if `method` is a function and support parameter validation?
          p5.errorLogged = _validateParams(method, arguments);
          method.apply(this, args);
          p5.erroLogged = false;
    }
}

// Somewhere else once only
// Need to double check this for loop can actually work
for(const [name, member] of Object.entries(p5)){
    validateParameterWrap(name, member);
}

To make this also available to external libraries, the validateParameterWrap will need to be slightly expanded to include parameter validation data that can be attached to be used by _validateParams. I hope the idea make sense, in any case there are some ideas I'm getting from this that I need to find some headspace to formalize (there's still some extra opportunity for nicer API that I'm feeling here) but if anyone else can do it that would be great as well.