1164297 - |MediaDecoderStateMachine| logging accesses env vars every time something is logged

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Eric Rahm [:erahm]]

|VERBOSE_LOG| [1] and |SAMPLE_LOG| [2] both use env vars to test whether they are enabled. Now that PR_LOGGING is always defined (and has been since Sept 2014) this could be problematic as |PR_GetEnv| acquires a lock for every call.

Env vars shouldn't change during a run, so we can probably cache the values during an init call.

A better option would be to:
  a) use a different log level for |VERBOSE_LOG|
  b) use a different logger for |SAMPLE_LOG|

[1] https://hg.mozilla.org/mozilla-central/annotate/617dbce26726/dom/media/MediaDecoderStateMachine.cpp#l62
[2] https://hg.mozilla.org/mozilla-central/annotate/617dbce26726/dom/media/MediaDecoderStateMachine.cpp#l68

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

I think Eric's suggestion makes sense as we already have MSE_DEBUGV whose log level is |PR_LOG_DEBUG + 1|(https://hg.mozilla.org/mozilla-central/file/617dbce26726/dom/media/mediasource/MediaSourceReader.cpp#l29).

Thoughts?

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Option A; PR_LOG_DEBUG+1.

I understand the long term plan is to move away from NSPR logging, since we're not able to make changes to it, so we should just do whatever we want here now.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

SAMPLE_LOG should also deserve its own log module, right? E.g, PR_NewLogModule("MediaSample")...

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1164297_dont_use_env-v1.patch

Per suggestion of comment 0.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8605026 [details] [diff] [review]
1164297_dont_use_env-v1.patch

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

Please send an email to mozilla.dev.media so our developers know about this change.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Sure. Thanks for the review.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21401957b641

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to JW Wang [:jwwang] from comment #3)
> SAMPLE_LOG should also deserve its own log module, right? E.g,
> PR_NewLogModule("MediaSample")...

Yeah, or being PR_lOG_DEBUG+2 even.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/21401957b641