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

ffmpeg: Improve Cgo logging #214

Merged
merged 2 commits into from
Nov 30, 2020
Merged

ffmpeg: Improve Cgo logging #214

merged 2 commits into from
Nov 30, 2020

Conversation

jailuthra
Copy link
Contributor

Why
Clean up logging in the Cgo code (Fixes #209)

What

  • Use ffmpeg's logging library with appropriate levels like ERROR, WARNING, INFO instead of throwing everything on stderr blindly.
  • Add a well defined log prefix like [lpms.decoder.xyz] of the component from where the log is being emitted.
  • Does not add any extra INFO logs, will do it separately the next time I go around debugging something and need to trace out the location in the transcoder pipeline

Notes
Commit(s) to be applied on top of #212

@oscar-davids
Copy link
Contributor

I think it's a must for code maintenance and debugging.

As we know, ffmpeg logging is leveled by the "av_log_set_level" function. But this function has been hard-coded as "AV_LOG_WARNING" in our C level lpms_init() function.
I think it would be better to redefine the C level lpms_init() function and the go level InitFFmpeg() function for outputting the log.

type LogLevel int

const (
LogPanic LogLevel = iota
LogFatal
LogError
LogWarning
LogInfo
LogVerbose
LogDebug
LogTrace
)

func InitFFmpeg(level LogLevel ) {
...
}

Along with this, I think that if some stuff discussed in #212 would be supplemented, it would be a better development environment.

@jailuthra
Copy link
Contributor Author

But this function has been hard-coded as "AV_LOG_WARNING" in our C level lpms_init() function.

@oscar-davids Ah yes I had thought about making that match the golang's glog logging level but left it as-is because matching the levels might lead to really verbose ffmpeg logs.

But the suggestion you make around making it separately configurable using a golang enum sounds like a good way to go! Will do this in the update commit.

I think it would be better to redefine the C level lpms_init() function and the go level InitFFmpeg() function for outputting the log.

@jailuthra
Copy link
Contributor Author

Updates:

  • Create shared #define macros for the logging functions for ERROR, WARNING and INFO levels
  • Make logging level configurable via the C and Golang module's init() methods

@jailuthra jailuthra changed the base branch from jai/reorg to master November 30, 2020 14:07
@jailuthra jailuthra merged commit 967c187 into master Nov 30, 2020
@jailuthra jailuthra deleted the jai/logs branch December 2, 2020 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Logging
2 participants