Remove MediaDecoderStateMachine::mAudioEndTime - first

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
Depends on: 1182738
Try looks green.
Bug 1183007. Part 1 - provide a wrapper function so that all read from mAudioEndTime must be through MDSM::AudioEndTime().
Attachment #8633373 - Flags: review?(kinetik)
Bug 1183007. Part 2 - remove dependency on MDSM::OnAudioEndTimeUpdate from AudioSink.
Attachment #8633374 - Flags: review?(kinetik)
Bug 1183007. Part 3 - remove dependency on MDSM::OnAudioEndTimeUpdate from DecodedStream.
Attachment #8633375 - Flags: review?(roc)
Bug 1183007. Part 4 - remove code that is not used anymore.
Attachment #8633376 - Flags: review?(roc)
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 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\!
Attachment #8633373 - Flags: review?(kinetik) → review+
Attachment #8633374 - Flags: review?(kinetik) → review+
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.
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 on attachment 8633375 [details]
MozReview Request: Bug 1183007. Part 2.5 - don't call AudioEndTime() after mAudioSink becomes null.

Cancel review per comment 10.
Attachment #8633375 - Flags: review?(roc)
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.
Attachment #8633376 - Flags: review?(roc)
Summary: Remove MediaDecoderStateMachine::mAudioEndTime → Remove MediaDecoderStateMachine::mAudioEndTime - first
Let's deal with AudioSink only in this bug. I will file another bug to handle DecodedStream and remove mAudioEndTime.
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().
Attachment #8633373 - Flags: review+ → review?(kinetik)
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.
Attachment #8633374 - Flags: review+ → review?(kinetik)
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.
Attachment #8633375 - Attachment description: MozReview Request: Bug 1183007. Part 3 - remove dependency on MDSM::OnAudioEndTimeUpdate from DecodedStream. → MozReview Request: Bug 1183007. Part 2.5 - don't call AudioEndTime() after mAudioSink becomes null.
Attachment #8633375 - Flags: review?(kinetik)
Attachment #8633376 - Attachment is obsolete: true
Attachment #8633376 - Attachment is patch: true
Attachment #8633376 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8633373 - Flags: review?(kinetik) → review+
Attachment #8633374 - Flags: review?(kinetik) → review+
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!)
Attachment #8633375 - Flags: review?(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!
Attachment #8633375 - Flags: review+
(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!
Blocks: 1184412
Assignee: nobody → jwwang
You need to log in before you can comment on or make changes to this bug.