1178437 - Fix up the remaining handful of methods in MediaDecoderStateMachine that rely on the monitor

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

We're very close now.

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Assert OnTaskQueue for most of the remaining MDSM methods. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove ReadOnWrongThread. v1

\o/

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Dispatch SetFragmentEndTime. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Make mRealTime const and allow it to be accessed on any thread. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Do the dormant-enabled tracking on the main thread. v1

This is purely a question of policy, so there's no reason it needs to live on
the off-main-thread decoding machinery.

Comment 7

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

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.

Comment 8

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

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.

Comment 9

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

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;
}

Comment 10

[Jean-Yves Avenard [:jya]]

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

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Jean-Yves Avenard [:jya]]

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

Comment 13

[Pulsebot]

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

Comment 14

[Carsten Book [:Tomcat]]

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