Closed Bug 816741 Opened 7 years ago Closed 7 years ago

Add a callback function in WebMReader for nestegg logging

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: sworkman, Assigned: sworkman)

Details

Attachments

(1 file, 1 obsolete file)

Nestegg provides a callback parameter for a logging function. In order to know more of what is being decoded in nestegg, such a function needs to be implemented in WebMReader.
Adds a basic logging function to WebMReader, with "%p [Nestegg]" at the start, where %p is the pointer address of the nestegg context structure. Logging is attached to gMediaDecoderLog, i.e. enabled with NSPR_LOG_MODULE="MediaDecoder:5,...".
Attachment #686820 - Flags: review?(kinetik)
Comment on attachment 686820 [details] [diff] [review]
v1.0 Add nestegg logging function to WebMReader

Review of attachment 686820 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webm/WebMReader.cpp
@@ +104,5 @@
>  
> +#ifdef PR_LOGGING
> +static void webm_log(nestegg * context,
> +                     unsigned int severity,
> +                     char const * format, ...)

Declare this unconditionally, and just have the body conditioned on PR_LOGGING.

@@ +113,5 @@
> +  va_start(args, format);
> +
> +  PR_snprintf(msg, sizeof(msg), "%p [Nestegg] ", context);
> +  PR_vsnprintf(msg+strlen(msg), sizeof(msg)-strlen(msg), format, args);
> +  PR_LOG(gMediaDecoderLog, PR_LOG_DEBUG, (msg));

How verbose is this in practice?  Maybe we should add a new demuxer/codec "detail" log target (that can be shared with the other backends) to avoid cluttering the current log target?

@@ +254,5 @@
>  #else
>    int64_t maxOffset = -1;
>  #endif
> +#ifdef PR_LOGGING
> +  int r = nestegg_init(&mContext, io, &webm_log, maxOffset);

...that way, we don't need a conditional here at all.
Attachment #686820 - Flags: review?(kinetik) → review+
(In reply to Matthew Gregan [:kinetik] from comment #2)
> Comment on attachment 686820 [details] [diff] [review]
> v1.0 Add nestegg logging function to WebMReader
> 
> Review of attachment 686820 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review! I added a switch statement to include a 3 letter string based on the log severity, e.g. "ERR" or "DBG". I'll carry the r+ forward on the patch, but you can let me know if you think there's a problem with that.

> ::: content/media/webm/WebMReader.cpp
> @@ +104,5 @@
> >  
> > +#ifdef PR_LOGGING
> > +static void webm_log(nestegg * context,
> 
> Declare this unconditionally, and just have the body conditioned on
> PR_LOGGING.
> @@ +254,5 @@
> > +#ifdef PR_LOGGING
> > +  int r = nestegg_init(&mContext, io, &webm_log, maxOffset);
> 
> ...that way, we don't need a conditional here at all.

Done.

> @@ +113,5 @@
> > +  PR_LOG(gMediaDecoderLog, PR_LOG_DEBUG, (msg));
> 
> How verbose is this in practice?  Maybe we should add a new demuxer/codec
> "detail" log target (that can be shared with the other backends) to avoid
> cluttering the current log target?

So, I see that there is already an OmxDecoder log variable. So, rather than added a generic one for all decoder backends, I just added gNesteggLog in WebMReader.cpp.
Attachment #686820 - Attachment is obsolete: true
Attachment #692118 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f3de630ebd0d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.