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

Have the Untrunc executable provide more help #125

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Hacklin
Copy link
Contributor

@Hacklin Hacklin commented Jul 1, 2018

  • Supply reason why a command-line argument was incorrect.
  • Add help option (-h).
  • Add verbosity options (-q, -v) to set the AV library's log level.

@Hacklin Hacklin changed the title Provide more help. Provide more help by the Untrunc executable Jul 2, 2018
@Hacklin Hacklin changed the title Provide more help by the Untrunc executable Have the Untrunc executable provide more help Jul 2, 2018
@anthwlock
Copy link

Hey, I am not the maintainer but I think you add more complexity than needed. Keep in mind that this program is only to be used for repairing mp4 files. You don't need 10 different log levels for this. I like the KISS priciple.

@Hacklin
Copy link
Contributor Author

Hacklin commented Aug 2, 2018

Untrunc doesn't only repair files;
it also allows you to analyze a file (option -a).
Changing the log level allows you to change the presented info by FFmpeg/Libav,
when analyzing a file.

The log levels are the ones used in FFmpeg and Libav.
If you look at Untrunc as an FFmpeg FFtool or a Libav AvTool,
then Untrunc should support these log levels, just as all FFtools/AvTools do.
If you look at Untrunc as its own little util,
then we could remove some of them.
Log levels:

  • quiet: For FFmpeg/Libav this means none as in no logging at all.
    Could map this to fatal or error.
    Note that quiet does not mean completely silent.
  • panic: Unrecoverable hardware error.
    I do not see the point in this log level for pure software, that doesn't deal with hardware directly.
    So, this level should be removed.
  • fatal: Unrecoverable software error.
    C++ exception in Untrunc fall in this category.
    Untrunc uses exceptions, a lot.
    Actually, Untrunc uses exceptions wrong; e.g. it throws exceptions for invalid data.
    Validation is a normal (routine) job of any program, and does not warrant exceptions.
    I think, just about all exceptions in Untrunc should be removed.
    This log level could be removed; i.e. mapped to error.
  • error, warning, info: Pretty standard.
    Useful, keep them.
  • verbose: Even more info.
    The additional info from FFmpeg/Libav can be useful.
    I like to keep this.
  • debug: For debugging the program itself.
    Normally, the added info that debug should provide,
    is to help with bug reports against the program itself.
  • trace: Function call tracing, parse tracing.
    This log level is normally not in release versions, due to the added overhead.
    Then again, Untrunc is too small to do separate release and debug builds.
    Untrunc does not really do this. Except may for a bit in Codec::matchSample().
    This log level could be removed; i.e. mapped to debug.
    But then, do you map Untrunc debug to FFmpeg/Libav debug or to FFmpeg/Libav trace?
  • Hidden log levels.
    You could reduce the perceived number of log levels by not showing them in the help.
    This does help by not overwhelming end-users.

Please, tell me how you would map the Untrunc log levels to FFmpeg/Libav ones?

@anthwlock
Copy link

I think one loglevel should be dedicated to the the average user, who doesn't know a lot about MP4s. But sometimes untrunc does not work or you are a developer. For this we need a verbose loglevel. So in my opinion two loglevels should be enough. I'd focus on that first, and if later for some reason an additional loglevel makes sense, just add it then.

For the loglevel dedicated to the average user I think AV_LOG_WARNING is ok.

@Hacklin
Copy link
Contributor Author

Hacklin commented Aug 4, 2018

The code has quite some logging in #ifdef VERBOSE sections.
Do all that #ifdef VERBOSE logging belong in the debug log level?

Hacklin added 3 commits August 5, 2018 10:31
Alter both `enum LogLevel` and `struct LogInfo`
to get the desired logging levels.
@anthwlock
Copy link

Neither verbose nor debug messages are of interest to the average user. So I'd put them into the same log level.

@Hacklin
Copy link
Contributor Author

Hacklin commented Aug 14, 2018

@anthwlock
In your fork, you use the log levels/modes: E, W, I, V, VV.
Could you describe when to use Verbose (V) and VeryVerbose (VV) ?

@Hacklin
Copy link
Contributor Author

Hacklin commented Aug 14, 2018

Logging (Severity) Levels:

  • Programming
    All logging levels should be available for use and they should follow some "standard".
    This allows a programmer concentrate on the code,
    instead of thinking which log levels do what.
  • Log Module
    The Log Module determines which logging levels actually log when.
    It can merge logging levels into a single level.
    It can remove a logging level by turning it into a no-op.
    This allows the log output to change without affecting the sources.
  • Log Configuration by User
    What the user is allowed to configure.
    Normally, this is everything that the Log Module allows,
    but you can have hidden (debugging) logging levels.

Some "standard" logging levels:

  • Linux: emerg [=panic], alert, crit, err, warning, notice, info, debug.
  • Syslog: emerg [=panic], alert, crit, err, warning, notice, info, debug.
  • Apache: (off), fatal, error, warn, info, debug, trace.
  • Boost: fatal, error, warning, info, debug, trace.
  • IBM Base Events: Fatal, Critical, Minor, Warning, Harmless, Information.

Note:

  • I assume that there are no Log Classes/Facilities;
    i.e. the entire program is in the same log class.

@anthwlock
Copy link

I used VV for the output which is not directly related to untrunc or MP4s. For example output about the buffer in FileRead is VV.

@Hacklin
Copy link
Contributor Author

Hacklin commented Aug 14, 2018

Hmmm ,
In file.cpp you use VV like Trace,
but in nal-slice.cpp you use VV for user output for -a option.

And I see that you use V like Debug.

@anthwlock
Copy link

Well, then let's say I use VV when I feel like it's too verbose for V. Tbh I didn't take too much time to think about which channel I put what message in. I only implemented this logging sytem so the average user does not get bombarded by messages he doesn't understand.

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.

2 participants