1162381 - Remove mAudioCaptured exemption for GetClock assertion

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

See bug 1161901 comment 8. I'm relaxing the assertion slightly in bug 1161901, since that bug makes bug 1161816 perma-orange.

Comment 1

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

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

Comment 2

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

Attachment: 1162381_part1_fix_mNextVideoTime-v1.patch

Comment 3

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

Comment on attachment 8615841 [details] [diff] [review]
1162381_part1_fix_mNextVideoTime-v1.patch

Review of attachment 8615841 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +577,5 @@
>          stream->mHaveSentFinishVideo = true;
>        }
>        endPosition = std::max(endPosition,
>            mediaStream->MicrosecondsToStreamTimeRoundDown(
> +              stream->mNextVideoTime - stream->mInitialTime - mStartTime));

This is a minor bug. We should include |mStartTime| when calculating video end position.

Comment 4

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

Attachment: 1162381_part2_refactor_DecodedStreamData_mInitialTime-v1.patch

Comment 5

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

Comment on attachment 8615843 [details] [diff] [review]
1162381_part2_refactor_DecodedStreamData_mInitialTime-v1.patch

Review of attachment 8615843 [details] [diff] [review]:
-----------------------------------------------------------------

Remove DecodedStreamData::mInitialTime so that the position is calculated in the same way as AudioSink. One day we will unify DecodedStream and AudioSink which in a high level sense are both audio/video data consumers.

::: dom/media/DecodedStream.h
@@ +41,3 @@
>    ~DecodedStreamData();
>    bool IsFinished() const;
> +  int64_t GetPosition() const;

Change the function name to be consistent with that of AudioSink::GetPosition.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2922,5 @@
>      } else {
>        // Audio is disabled on this system. Sync to the system clock.
>        clock_time = GetVideoStreamPosition();
>      }
> +    NS_ASSERTION(GetMediaTime() <= clock_time || mPlaybackRate <= 0,

Now we can remove the exception of mAudioCaptured from the assertion.

@@ +3522,5 @@
> +      self->StopAudioThread();
> +      // GetMediaTime() could return -1 because we haven't decoded
> +      // the 1st frame. But this is OK since we will update mStreamStartTime
> +      // again in SetStartTime().
> +      self->mStreamStartTime = self->GetMediaTime();

This becomes tricky when we want to switch to capture mode in the middle of playback. Therefore I decide to do the switch outside the state machine cycles to make things easier.

::: dom/media/MediaDecoderStateMachine.h
@@ +1001,5 @@
>    AbstractCanonical<int64_t>* CanonicalCurrentPosition() { return &mCurrentPosition; }
>  protected:
> +  // The presentation time of the first audio/video frame that is sent to the
> +  // media stream.
> +  int64_t mStreamStartTime;

We will remove this member once we are able to split DecodedStream into AudioSink and VideoSink and use mAudioStartTime instead.

Comment 6

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

Thanks for the review!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/212ed262e146
https://hg.mozilla.org/integration/mozilla-inbound/rev/be7d64ddda20

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/212ed262e146
https://hg.mozilla.org/mozilla-central/rev/be7d64ddda20