Closed Bug 1178437 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files)

We're very close now.
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.
Blocks: 1178938
\o/
Attachment #8627993 - Flags: review?(jwwang)
Attachment #8627994 - Flags: review?(jwwang)
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)
Attachment #8627991 - Flags: review?(jwwang) → review+
Attachment #8627993 - Flags: review?(jwwang) → review+
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 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 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+
(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.
(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 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+
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.