1183007 - Remove MediaDecoderStateMachine::mAudioEndTime - first

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

As bug 1182738, we will be able to remove MediaDecoderStateMachine::mAudioEndTime after bug 1182738 is landed.

This also removes the need for MediaDecoderStateMachine::OnAudioEndTimeUpdate() and reduces the bi-directional dependency between MDSM and AudioSink.

Another step forward to unify AudioSink and DecodedStream which are both consumers of media data.

Comment 1

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=563c98580429

Comment 2

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

Try looks green.

Comment 3

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

Attachment: MozReview Request: Bug 1183007. Part 1 - provide a wrapper function so that all read from mAudioEndTime must be through MDSM::AudioEndTime().

Bug 1183007. Part 1 - provide a wrapper function so that all read from mAudioEndTime must be through MDSM::AudioEndTime().

Comment 4

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

Attachment: MozReview Request: Bug 1183007. Part 2 - remove dependency on MDSM::OnAudioEndTimeUpdate from AudioSink.

Bug 1183007. Part 2 - remove dependency on MDSM::OnAudioEndTimeUpdate from AudioSink.

Comment 5

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

Attachment: MozReview Request: Bug 1183007. Part 2.5 - don't call AudioEndTime() after mAudioSink becomes null.

Bug 1183007. Part 3 - remove dependency on MDSM::OnAudioEndTimeUpdate from DecodedStream.

Comment 6

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

Attachment: MozReview Request: Bug 1183007. Part 4 - remove code that is not used anymore.

Bug 1183007. Part 4 - remove code that is not used anymore.

Comment 7

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

https://reviewboard.mozilla.org/r/13209/#review11811

::: dom/media/DecodedStream.h:114
(Diff revision 1)
> -  CheckedInt64 AudioEndTime() const;
> +  int64_t AudioEndTime() const;

Return int64_t so it is consistent with AudioSink::GetEndTime().

::: dom/media/DecodedStream.cpp:635
(Diff revision 1)
> -  MOZ_ASSERT(mStartTime.isSome(), "Must be called after StartPlayback()");
> +  if (mStartTime.isSome() && mInfo.HasAudio()) {

Now this could be called before StartPlayback(). Return -1 to indicate there is no audio end time.

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8633373 [details]
MozReview Request: Bug 1183007. Part 1 - provide a wrapper function so that all read from mAudioEndTime must be through MDSM::AudioEndTime().

https://reviewboard.mozilla.org/r/13205/#review11817

Ship It\!

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8633374 [details]
MozReview Request: Bug 1183007. Part 2 - remove dependency on MDSM::OnAudioEndTimeUpdate from AudioSink.

https://reviewboard.mozilla.org/r/13207/#review11819

Ship It!

::: dom/media/MediaDecoderStateMachine.cpp:3077
(Diff revision 1)
> +    return mAudioSink->GetEndTime();

Should this update mAudioEndTime? It seems wrong for AudioEndTime() to return a different value after mAudioSink becomes null, if AudioEndTime() is ever called after mAudioSink becomes null.

Comment 10

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

https://reviewboard.mozilla.org/r/13207/#review11819

> Should this update mAudioEndTime? It seems wrong for AudioEndTime() to return a different value after mAudioSink becomes null, if AudioEndTime() is ever called after mAudioSink becomes null.

True that this is indeed a problem. Updating mAudioEndTime each time AudioEndTime() is called is not perfect since there will be a small error between the audio end time observed by MDSM and the true auido end time kept by AudioSink. I am trying to figure out if it makes sense to return mDecodedAudioEndTime when mAudioCompleted is true.

Comment 11

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

Comment on attachment 8633375 [details]
MozReview Request: Bug 1183007. Part 2.5 - don't call AudioEndTime() after mAudioSink becomes null.

Cancel review per comment 10.

Comment 12

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

Comment on attachment 8633376 [details] [diff] [review]
MozReview Request: Bug 1183007. Part 4 - remove code that is not used anymore.

Cancel review per comment 10.

Comment 13

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

Try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0332fed29d38

Comment 14

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

Let's deal with AudioSink only in this bug. I will file another bug to handle DecodedStream and remove mAudioEndTime.

Comment 15

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

Comment on attachment 8633373 [details]
MozReview Request: Bug 1183007. Part 1 - provide a wrapper function so that all read from mAudioEndTime must be through MDSM::AudioEndTime().

Bug 1183007. Part 1 - provide a wrapper function so that all read from mAudioEndTime must be through MDSM::AudioEndTime().

Comment 16

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

Comment on attachment 8633374 [details]
MozReview Request: Bug 1183007. Part 2 - remove dependency on MDSM::OnAudioEndTimeUpdate from AudioSink.

Bug 1183007. Part 2 - remove dependency on MDSM::OnAudioEndTimeUpdate from AudioSink.

Comment 17

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

Comment on attachment 8633375 [details]
MozReview Request: Bug 1183007. Part 2.5 - don't call AudioEndTime() after mAudioSink becomes null.

Bug 1183007. Part 2.5 - don't call AudioEndTime() after mAudioSink becomes null.

Comment 18

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

Patch 2.5 addresses the issues in comment 10.
Try is green. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0332fed29d38

Comment 19

[Matthew Gregan [:kinetik]]

Comment on attachment 8633375 [details]
MozReview Request: Bug 1183007. Part 2.5 - don't call AudioEndTime() after mAudioSink becomes null.

https://reviewboard.mozilla.org/r/13209/#review11953

::: dom/media/MediaDecoderStateMachine.cpp:2440
(Diff revision 2)
> +      // is wrong to returns -1 for mAudioSink is null.

My brain's tired, but I'm struggling to parse the comment a bit.  Would you remind rewording it to something like the following?

"Stop audio sink after call to AudioEndTime() above, otherwise it will return an incorrect value due to a null mAudioSink."

(assuming my interpretation is correct!)

Comment 20

[Matthew Gregan [:kinetik]]

Comment on attachment 8633375 [details]
MozReview Request: Bug 1183007. Part 2.5 - don't call AudioEndTime() after mAudioSink becomes null.

https://reviewboard.mozilla.org/r/13209/#review11955

Ship It!

Comment 21

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

(In reply to Matthew Gregan [:kinetik] from comment #19)
> Comment on attachment 8633375 [details]
> MozReview Request: Bug 1183007. Part 2.5 - don't call AudioEndTime() after
> mAudioSink becomes null.
> 
> https://reviewboard.mozilla.org/r/13209/#review11953
> 
> ::: dom/media/MediaDecoderStateMachine.cpp:2440
> (Diff revision 2)
> > +      // is wrong to returns -1 for mAudioSink is null.
> 
> My brain's tired, but I'm struggling to parse the comment a bit.  Would you
> remind rewording it to something like the following?
> 
> "Stop audio sink after call to AudioEndTime() above, otherwise it will
> return an incorrect value due to a null mAudioSink."
> 
> (assuming my interpretation is correct!)

Sure, I will rephrase the comment as suggested. Thanks for the review!

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/492e9a6065d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/6db05cc790a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/fafc65c08458

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/492e9a6065d2
https://hg.mozilla.org/mozilla-central/rev/6db05cc790a9
https://hg.mozilla.org/mozilla-central/rev/fafc65c08458