Closed
Bug 1178437
Opened 9 years ago
Closed 9 years ago
Fix up the remaining handful of methods in MediaDecoderStateMachine that rely on the monitor
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(5 files)
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
jya
:
review+
|
Details | Diff | Splinter Review |
We're very close now.
Assignee | ||
Comment 1•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4bced27fddb
This is green except for W-4, which exhibits the same failure mode when I do a local build of the base revision. Presuming unrelated.
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8627991 -
Flags: review?(jwwang)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8627994 -
Flags: review?(jwwang)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8627995 -
Flags: review?(jwwang)
Assignee | ||
Comment 6•9 years ago
|
||
This is purely a question of policy, so there's no reason it needs to live on
the off-main-thread decoding machinery.
Attachment #8627996 -
Flags: review?(jwwang)
Updated•9 years ago
|
Attachment #8627991 -
Flags: review?(jwwang) → review+
Updated•9 years ago
|
Attachment #8627993 -
Flags: review?(jwwang) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8627994 [details] [diff] [review]
Part 3 - Dispatch SetFragmentEndTime. v1
Review of attachment 8627994 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/MediaDecoderStateMachine.h
@@ +309,5 @@
> + void DispatchSetFragmentEndTime(int64_t aEndTime)
> + {
> + nsRefPtr<MediaDecoderStateMachine> self = this;
> + nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction([self, aEndTime] () {
> + self->mFragmentEndTime = aEndTime < 0 ? aEndTime : aEndTime;
This is equivalent to |self->mFragmentEndTime = aEndTime|. The original code is weird.
Attachment #8627994 -
Flags: review?(jwwang) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8627995 [details] [diff] [review]
Part 4 - Make mRealTime const and allow it to be accessed on any thread. v1
Review of attachment 8627995 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/MediaDecoderStateMachine.h
@@ +726,5 @@
> WatchManager<MediaDecoderStateMachine> mWatchManager;
>
> + // True is we are decoding a realtime stream, like a camera stream. Immutable
> + // after construction.
> + const bool mRealTime;
All const members are immutable after construction by its definition. We don't need to add a comment for that.
Attachment #8627995 -
Flags: review?(jwwang) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8627996 [details] [diff] [review]
Part 5 - Do the dormant-enabled tracking on the main thread. v1
Review of attachment 8627996 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/mediasource/SourceBufferContentManager.cpp
@@ +38,5 @@
> + aParentDecoder->NotifyDormantSupported(Preferences::GetBool("media.decoder.heuristic.dormant.enabled", false));
> + }
> +#endif
> +
> +
Overall looks good to me. But I think jya should take a look at the MSE part.
I am not sure if this change is equivalent to
bool
MediaSourceReader::IsDormantNeeded()
{
ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
if (GetVideoReader()) {
return GetVideoReader()->IsDormantNeeded();
}
return false;
}
Attachment #8627996 -
Flags: review?(jyavenard)
Attachment #8627996 -
Flags: review?(jwwang)
Attachment #8627996 -
Flags: review+
Comment 10•9 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #9)
> Comment on attachment 8627996 [details] [diff] [review]
> Part 5 - Do the dormant-enabled tracking on the main thread. v1
>
> Review of attachment 8627996 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/media/mediasource/SourceBufferContentManager.cpp
> @@ +38,5 @@
> > + aParentDecoder->NotifyDormantSupported(Preferences::GetBool("media.decoder.heuristic.dormant.enabled", false));
> > + }
> > +#endif
> > +
> > +
>
> Overall looks good to me. But I think jya should take a look at the MSE part.
>
> I am not sure if this change is equivalent to
>
> bool
> MediaSourceReader::IsDormantNeeded()
> {
> ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> if (GetVideoReader()) {
> return GetVideoReader()->IsDormantNeeded();
> }
>
> return false;
> }
it isn't...
On one hand you ask the sub-readers if dormant is supported (typically yes as we only support mp4 and all h264 decoders now support dormant) ; vs asking the MediaSourceDecoder which has no idea what the sub-readers are.
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #10)
> it isn't...
>
> On one hand you ask the sub-readers if dormant is supported (typically yes
> as we only support mp4 and all h264 decoders now support dormant) ; vs
> asking the MediaSourceDecoder which has no idea what the sub-readers are.
This change explicitly makes the MediaSourceDecoder compute dormant support in exactly the same was as the subdecoders do now (see NotifyDormantSupported). What is the behavior change you're concerned about?
Flags: needinfo?(jyavenard)
Comment 12•9 years ago
|
||
Comment on attachment 8627996 [details] [diff] [review]
Part 5 - Do the dormant-enabled tracking on the main thread. v1
Review of attachment 8627996 [details] [diff] [review]:
-----------------------------------------------------------------
I don't like it.
This moves the decision to decide if we support dormant or not to the decoder causes the container to decide if dormant is supported or not..
Anything using the MediaFormatReader is dormant capable.
::: dom/media/mediasource/SourceBufferContentManager.cpp
@@ +32,5 @@
> +
> + // Now that we know what type we're dealing with, enable dormant as needed.
> +#if defined(MP4_READER_DORMANT_HEURISTIC)
> + if (aType.LowerCaseEqualsLiteral("video/mp4") ||
> + aType.LowerCaseEqualsLiteral("audio/mp4"))
|| useFormatReader
Attachment #8627996 -
Flags: review?(jyavenard) → review+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/617f5838432f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a32fc2b22f7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/852e6e48ada6
https://hg.mozilla.org/integration/mozilla-inbound/rev/a641cf86fa27
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddb5d638fcbf
Updated•9 years ago
|
Flags: needinfo?(jyavenard)
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/617f5838432f
https://hg.mozilla.org/mozilla-central/rev/a32fc2b22f7d
https://hg.mozilla.org/mozilla-central/rev/852e6e48ada6
https://hg.mozilla.org/mozilla-central/rev/a641cf86fa27
https://hg.mozilla.org/mozilla-central/rev/ddb5d638fcbf
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•