WebCanvas: Calling animation.frame(0) before play() throws an error

[zubiden]

While working on the frame caching demo, I noticed that any Animation call resulting in an internal seek to frame 0 before the play() call throws the following error:

TVGERR - frame (set) failed: Insufficient condition

Reproducible on https://thorvg-playground.vercel.app/showcase/lottie-animation. Add animation.frame(0), animation.seek(0) or animation.stop() before line 34.

Setting the frame to 0 after the play() call works fine.

[tinyjin]

Thanks @zubiden.
I wasn't able to reproduce by changing code before line 34. Could you confirm that my screenshot is correct to reproduce your situation?

[hermet]

Please note that the native API will not return success if the frame is already set. This allows users to skip the canvas update/render when the frame does not need to be changed.

if (animation->frame(x) == success) {
    canvas->update();
    canvas->render();
} else {
    //requested the same frame animation. no need to redraw.
}

[zubiden]

Thanks @zubiden. I wasn't able to reproduce by changing code before line 34. Could you confirm that my screenshot is correct to reproduce your situation?

Sorry, that was badly worded. It doesn't throw an error there, but it does log one, since you have an error handler

thorvg.web/playground/components/CanvasPreview.tsx

Line 43 in c1b4e73

onError: (error, context) => {

Without an error handler, it throws. I assume this should not happen if it is an expected behavior?

[tinyjin]

@zubiden Hello. As you understand, when you have an ErrorHandler, the error will be handled there. Otherwise, it will throw a system error(You can use this style in debug mode for simpler error handling.)

This is the expected behavior. We recommend using ErrorHandler in a production environment to avoid excessive throws, to handle user-expectidly.

WebCanvas follows ThorVG error types. Thus note, ThorVGResultCode is just passed to applications when it's not ThorVGResultCode.Success, devs can choose whether they ignore or handle.

You can have error handler like this to handle in this case.

if (context.operation === 'frame (set)' && error.code === ThorVGResultCode.InsufficientCondition) {
  // ignore as expected
  return;
}

// handle another erros

[tinyjin]

We might need to explicit enumerations for each API operations, so we can implement api-specific handling logic, rather than relying on string type. (Also Paint object that causes error in ErrorContext)

/** The operation that failed (e.g., 'moveTo', 'render', 'update') */
operation: string; // current

In addition, documenting special error types in the WebCanvas docs, including special error types such as frame(), so application devs can understand without looking up native API docs, and handle correctly.

• Animation::frame()
• Paint::rotate(), scale(), translate()
• Initializer::term()

These all return InsufficientCondition when they're already applied.

[zubiden]

Otherwise, it will throw a system error

I find it confusing that library performing a 'noop' results in an error that I need to handle... by ignoring it. While it is great that it provides a result code to avoid rendering, it should not be accompanied by an exception in this case.

You can have error handler like this to handle in this case.

The issue with this approach is that it requires me to be familiar with the inner workings of the library to effectively handle errors. Had I not checked the code, I would not have known that 'seek' and 'stop' could throw the same error, which could potentially cause an issue in a production environment.

I would also prefer to avoid global error handlers and use the try...catch to maintain context and attempt recovery. In my opinion, it is unnecessary to raise an exception if the system is working perfectly.

I suggest handling such cases on the library side, to avoid useless exceptions for the library users. seek would probably need to return result code to be consistent with frame. Calling stop without try...catch feels like Russian roulette right now 😟

[tinyjin]

@zubiden
Since the operation names are currently based on the native API, they may be ambiguous when it comes to handling behavior.

In my view, for functions such as seek, stop, and play (which are provided for usability rather than being part of the native API), it might be better to handle unnecessary errors internally for the sake of user convenience. I’ll take a closer look at this point.

For APIs like frame that are aligned with the native API, it still makes sense to keep the existing behavior in order to match the parent API’s error cases. However, I agree that we should improve the API documentation to make this behavior clearer.

Thank you for the feedback. I’ll take some time to review this more carefully.