Closed Bug 1241677 Opened 5 years ago Closed 5 years ago

add report on which PDM is being used in about:media

Categories

(Core :: Audio/Video: Playback, defect)

45 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jya, Assigned: jya)

Details

Attachments

(2 files, 1 obsolete file)

Will be useful to determine the stuff used to decode.
Comment on attachment 8710816 [details] [diff] [review]
P2. Add media decoder's description to about:media report.

sorry missed a portion of it
Attachment #8710816 - Attachment is obsolete: true
Attachment #8710816 - Flags: review?(cpearce)
Assignee: nobody → jyavenard
Summary: Had report on which PDM is being used in about:media → add report on which PDM is being used in about:media
Comment on attachment 8710815 [details] [diff] [review]
P1. Add MediaDataDecoder::GetDescriptionName() method.

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

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +195,5 @@
>    }
>  
> +  const char* GetDescriptionName() const override
> +  {
> +    return "blank media data decoder";

"android audio decoder"?

::: dom/media/platforms/apple/AppleVTDecoder.h
@@ +30,5 @@
> +  const char* GetDescriptionName() const override
> +  {
> +    return mIsHardwareAccelerated
> +      ? "apple hardware VT decoder"
> +      :  "apple software VT decoder";

Two spaces after ':'.
Attachment #8710815 - Flags: review?(cpearce) → review+
Comment on attachment 8710820 [details] [diff] [review]
P2. Add media decoder's description to about:media report.

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

::: dom/media/MediaFormatReader.cpp
@@ +1689,1 @@
>                              mVideo.mNumSamplesOutputTotal,

Why is it OK to assume that mNumSamplesOutputTotal can be accessed without a monitor but mDescription requires a monitor?

Given these are for informational purposes, it's not a huge deal I guess.
Attachment #8710820 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #6)
> Comment on attachment 8710820 [details] [diff] [review]
> P2. Add media decoder's description to about:media report.
> 
> Review of attachment 8710820 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaFormatReader.cpp
> @@ +1689,1 @@
> >                              mVideo.mNumSamplesOutputTotal,
> 
> Why is it OK to assume that mNumSamplesOutputTotal can be accessed without a
> monitor but mDescription requires a monitor?
> 
> Given these are for informational purposes, it's not a huge deal I guess.

None of this is perfectly thread-safe. about:media never have been

mNumSamplesOutputTotal is a 64 bits int; the other a char*

The consequence of a race reading a 64 bits int is at worse displaying rubbish for the current about:media, but reading a pointer and reading a C-string at that (possibly invalid) address is potentially far worse.

Ideally it should all be thread-safe, but I figured it's really just for our use.
https://hg.mozilla.org/mozilla-central/rev/13a942af5b86
https://hg.mozilla.org/mozilla-central/rev/4ea6060b3928
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.