816741 - Add a callback function in WebMReader for nestegg logging

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Add nestegg logging function to 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,...".

Comment 2

[Matthew Gregan [: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.

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Add nestegg logging function to WebMReader

(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.

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 692118 [details] [diff] [review]
v1.1 Add nestegg logging function to WebMReader

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=90cd7050c2bd

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3de630ebd0d

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f3de630ebd0d