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

[Impeller] Allow per call debug error checking and logging. #56632

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chinmaygarde
Copy link
Member

@chinmaygarde chinmaygarde commented Nov 15, 2024

In the OpenGL backend, the following behavior now applies:

If IMPELLER_DEBUG is unset (release runtime mode only):

  • Zero overhead during GL function dispatch. All flag checks and validations are compiled away.

If IMPELLER_DEBUG is set (debug and profile runtime modes only):

  • Debugging options are checked for error checking, call logging, and thread checking. Except thread checking for UseProgram, all options are false out of the box. The only overhead is the checking of the flags that are expected to be false.
  • Options for individual procs or the entire table can be toggled at runtime.

If IMPELLER_DEBUG is set and UNOPT modes are enabled:

  • Debug error checking is enabled out of the box. This is @gaaclarke s preference and matches previous behavior.

The difference between this and what is on ToT is that debugging is now available in OPT modes but the flags that enable checking are off by default (in OPT modes).

Wires up debugging options for the libImpeller.

This effectively backs out #54016

In the OpenGL backend, the following behavior now applies:

If IMPELLER_DEBUG is unset (release runtime mode only):
* Zero overhead during GL function dispatch.

If IMPELLER_DEBUG is set (debug and profile runtime modes only):
* Debugging options are checked for error checking, call logging, and thread checking. Except thread checking for UseProgram, all options are false out of the box.
* Options for individual procs or the entire table can be toggled at runtime.

If IMPELLER_DEBUG is set and UNOPT modes are enabled:
* Debug error checking is enabled out of the box. This is Aarons preference and matches previous behavior.

The difference between this and what is on ToT is that debugging is now available in OPT modes but the flags that enable checking are off by default (in OPT modes).

Wires up debugging options for the libImpeller.
@chinmaygarde
Copy link
Member Author

chinmaygarde commented Nov 15, 2024

I checked the overhead in the playground and could observe no difference (when the flags are all false out of the box of course). Its just checking the extra bools.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

How do you imagine a developer of the engine would turn on error checking for all of opengles?

/// @param[in] opengl_function_name The opengl function name
/// @param[in] enable The enable
///
IMPELLER_EXPORT void ImpellerContextSetDebugGLErrorChecking(
Copy link
Member

Choose a reason for hiding this comment

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

How do you expect these functions to be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Some instances from earlier this week:

  • We were trying to figure out why rendering after a save layer was broken. We enabled debugging all calls to bind and unbind framebuffer to discover that the framebuffer handle was not being delivered to Impeller. And, when that was fixed, Impeller was dropping the handle on the floor. So ImpellerContextSetDebugGLErrorChecking(context, "glBindFrambuffer", true)
  • I've used call logging to discover that texture initialization was nuking the contents that were provided by Android to Impeller without informing Impeller which then re-initialized the contents. This one was weird because RenderDoc doesn't understand the Android extensions.
  • The framebuffer binding issue manifested as an issue in the form a GL error on the first clear of the next frame. Enabling glGetError debugging after all calls led us to the bind framebuffer call in the first place.

Just some uses over the past week.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant how technically would someone who is developing use this to turn on debugging =). Do you want them to manually insert an invocation of this function somewhere? There are no callers to it in the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, they'd manually insert this during debugging using some sort of binding.

Copy link
Member

@gaaclarke gaaclarke Nov 16, 2024

Choose a reason for hiding this comment

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

I think that's going to be kind of hard to find/use. I think it would be easier if we just had an preprocessor macro for it.

// impeller/renderer/backend/gles/proc_table_gles.cc
#ifndef IMPELLER_LOG_GL_CALLS
  SetDebugGLCallLogging(true);
#endif  // IMPELLER_LOG_GL_CALLS
// If you want to just log messages a la carte you can use
// SetDebugGLCallLogging(true, "glClear");

Preprocessor macros aren't perfect but, proc table is the most logical place for someone to look for this. This way someone can get the behavior they want by just throwing in a #define IMPELLER_LOG_GL_CALLS without mucking around trying to find out where to put a call and how to call it with the right arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's going to be kind of hard to find/use.

This was for standalone Impeller embedders. The "engine" is usually a prebuilt and the embedder is smaller and can interactively enable this with bindings.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your note that there are no usages of this is valid though. Let me add an interop test too.

struct GLProc {
using GLFunctionType = T;

struct GLProcBase {
Copy link
Member

Choose a reason for hiding this comment

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

Why did this get split out?

Copy link
Member Author

Choose a reason for hiding this comment

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

The map that enable "lookup proc by name" needs an un-templated field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants