Closed Bug 1164297 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: erahm, Assigned: jwwang)

References

Details

Attachments

(1 file)

|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
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?
Flags: needinfo?(cpearce)
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.
Flags: needinfo?(cpearce)
SAMPLE_LOG should also deserve its own log module, right? E.g, PR_NewLogModule("MediaSample")...
Per suggestion of comment 0.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Attachment #8605026 - Flags: review?(cpearce)
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.
Attachment #8605026 - Flags: review?(cpearce) → review+
Sure. Thanks for the review.
(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.
https://hg.mozilla.org/mozilla-central/rev/21401957b641
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.