1282658 - Some AccurateSeekTask code cleanup

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Comment 1

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b6dd1d3f37d

Comment 2

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

Attachment: Bug 1282658. Part 1 - remove some null-checks. .

Review commit: https://reviewboard.mozilla.org/r/61440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61440/

Comment 3

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

Attachment: Bug 1282658. Part 2 - remove unnecessary checks for Exists().

1. On{Audio,Video}[Not]Decoded() is always called after Seek() and before
   mSeekTaskPromise is resolved or rejected. So mSeekTaskPromise should be
   never empty and Exists() should be always true.
2. Is{Audio,Video}SeekComplete is called by On{Audio,Video}[Not]Decoded()
   where Exists should be always true.

Review commit: https://reviewboard.mozilla.org/r/61442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61442/

Comment 4

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

Attachment: Bug 1282658. Part 3 - rename some functions. .

Review commit: https://reviewboard.mozilla.org/r/61444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61444/

Comment 5

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

Attachment: Bug 1282658. Part 4 - simplify logic of checking whether if audio/video seeking is completed. .

Review commit: https://reviewboard.mozilla.org/r/61446/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61446/

Comment 6

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

Attachment: Bug 1282658. Part 5 - remove unused members. .

Review commit: https://reviewboard.mozilla.org/r/61448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61448/

Comment 7

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

Attachment: Bug 1282658. Part 6 - remove unnecessary checks for |mSeekRequest.Exists()|. .

Ensure{Audio,Video}DecodeTaskQueued() is always called after OnSeekResolved()
where mSeekRequest is completed. So |mSeekRequest.Exists()| should be always
false in Ensure{Audio,Video}DecodeTaskQueued().

Review commit: https://reviewboard.mozilla.org/r/61450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61450/

Comment 8

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

Attachment: Bug 1282658. Part 7 - remove setting mNeedToStopPrerolling{Audio,Video} per discussion in https://reviewboard.mozilla.org/r/43689/#comment54421.

Review commit: https://reviewboard.mozilla.org/r/61452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61452/

Comment 9

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

Attachment: Bug 1282658. Part 8 - remove asserting |mReader->IsWaitForDataSupported()| for mReader->WaitForData() will crash if the method has no override.

Review commit: https://reviewboard.mozilla.org/r/61454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61454/

Comment 10

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

Attachment: Bug 1282658. Part 9 - return void for Ensure{Audio,Video}DecodeTaskQueued() for they always return NS_OK. .

Review commit: https://reviewboard.mozilla.org/r/61456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61456/

Comment 11

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

Attachment: Bug 1282658. Part 10 - merge Request{Audio,Video}Data() into Ensure{Audio,Video}DecodeTaskQueued() for the logs are not quite useful.

Review commit: https://reviewboard.mozilla.org/r/61458/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61458/

Comment 12

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

Attachment: Bug 1282658. Part 11 - merge OnAudioNotDecoded() and OnVideoNotDecoded().

Review commit: https://reviewboard.mozilla.org/r/61460/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61460/

Comment 13

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

Attachment: Bug 1282658. Part 12 - extract common code about adjusting fast seek.

Review commit: https://reviewboard.mozilla.org/r/61462/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61462/

Comment 14

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

Attachment: Bug 1282658. Part 13 - replace use of int64_t with media::TimeUnit.

Review commit: https://reviewboard.mozilla.org/r/61464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61464/

Comment 15

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

Attachment: Bug 1282658. Part 14 - remove mDrop{Audio,Video}UntilNextDiscontinuity for MediaData::mDiscontinuity is guaranteed to be true for the 1st sample after seeking.

Review commit: https://reviewboard.mozilla.org/r/61466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61466/

Comment 16

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

Attachment: Bug 1282658. Part 15 - optimize checking of seek complete.

We need to request another audio sample in OnAudioDecoded() only when
mDoneAudioSeeking is false which also applies OnVideoDecodd().
Therefore we remove calls to Ensure{Audio,Video}DecodeTaskQueued()
from CheckIfSeekComplete() to prevent requesting video samples inside
OnAudioDecoded() for they will be handled by OnVideoDecodd().

This also allows us to remove checking of mReader->IsRequesting{Audio,Video}Data()
from Ensure{Audio,Video}DecodeTaskQueued().

Review commit: https://reviewboard.mozilla.org/r/61468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61468/

Comment 17

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

Attachment: Bug 1282658. Part 16 - remove checks for |mReader->IsWaiting{Audio,Video}Data()|.

EnsureAudioDecodeTaskQueued() is called from several places where
mReader->IsWaitingAudioData() can be proven to be false:
1. OnAudioDecoded() - definitely false.
2. OnNotDecoded() - false because aReason is MediaDecoderReader::CANCELED.
3. OnSeekResolved() - false becuase we haven't requested any samples.
4. SetCallbacks() - false because AudioWaitCallback is resolved.

Review commit: https://reviewboard.mozilla.org/r/61470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61470/

Comment 18

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766617 [details]
Bug 1282658. Part 1 - remove some null-checks. .

https://reviewboard.mozilla.org/r/61440/#review58742

Comment 19

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766618 [details]
Bug 1282658. Part 2 - remove unnecessary checks for Exists().

https://reviewboard.mozilla.org/r/61442/#review58752

Comment 20

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766619 [details]
Bug 1282658. Part 3 - rename some functions. .

https://reviewboard.mozilla.org/r/61444/#review58754

Comment 21

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766620 [details]
Bug 1282658. Part 4 - simplify logic of checking whether if audio/video seeking is completed. .

https://reviewboard.mozilla.org/r/61446/#review58758

::: dom/media/AccurateSeekTask.cpp:514
(Diff revision 1)
>      EnsureAudioDecodeTaskQueued();
>      return;
>    }
>  
>    if (aReason == MediaDecoderReader::END_OF_STREAM) {
> -    mIsAudioQueueFinished = true;
> +    mDoneAudioSeeking = true;

We need to keep the "{audio,video} queue finished" information. So that we can notify the MDSM.

::: dom/media/AccurateSeekTask.cpp
(Diff revision 1)
>        // below.
>        mSeekedVideoData = mFirstVideoFrameAfterSeek;
>        mFirstVideoFrameAfterSeek = nullptr;
>      }
> -
> +    mDoneVideoSeeking = true;
> -    mIsVideoQueueFinished = true;

Same to the audio case.

Comment 22

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766621 [details]
Bug 1282658. Part 5 - remove unused members. .

https://reviewboard.mozilla.org/r/61448/#review58986

Comment 23

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766622 [details]
Bug 1282658. Part 6 - remove unnecessary checks for |mSeekRequest.Exists()|. .

https://reviewboard.mozilla.org/r/61450/#review58988

Comment 24

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766623 [details]
Bug 1282658. Part 7 - remove setting mNeedToStopPrerolling{Audio,Video} per discussion in https://reviewboard.mozilla.org/r/43689/#comment54421.

https://reviewboard.mozilla.org/r/61452/#review58968

::: dom/media/AccurateSeekTask.cpp
(Diff revision 1)
>      mReader->WaitForData(MediaData::AUDIO_DATA);
> -
> -    // We are out of data to decode and will enter buffering mode soon.
> -    // We want to play the frames we have already decoded, so we stop pre-rolling
> -    // and ensure that loadeddata is fired as required.
> -    mNeedToStopPrerollingAudio = true;

Then, mNeedToStopPrerollingAudio and mNeedToStopPrerollingVideo are not used anymore, they are not used in the NextFrameSeekTask. We could just remove them from the SeekTask.h.

Comment 25

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766624 [details]
Bug 1282658. Part 8 - remove asserting |mReader->IsWaitForDataSupported()| for mReader->WaitForData() will crash if the method has no override.

https://reviewboard.mozilla.org/r/61454/#review58970

Comment 26

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766625 [details]
Bug 1282658. Part 9 - return void for Ensure{Audio,Video}DecodeTaskQueued() for they always return NS_OK. .

https://reviewboard.mozilla.org/r/61456/#review58990

Comment 27

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766626 [details]
Bug 1282658. Part 10 - merge Request{Audio,Video}Data() into Ensure{Audio,Video}DecodeTaskQueued() for the logs are not quite useful.

https://reviewboard.mozilla.org/r/61458/#review58992

Comment 28

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766627 [details]
Bug 1282658. Part 11 - merge OnAudioNotDecoded() and OnVideoNotDecoded().

https://reviewboard.mozilla.org/r/61460/#review58994

Comment 29

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766628 [details]
Bug 1282658. Part 12 - extract common code about adjusting fast seek.

https://reviewboard.mozilla.org/r/61462/#review58996

Comment 30

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766629 [details]
Bug 1282658. Part 13 - replace use of int64_t with media::TimeUnit.

https://reviewboard.mozilla.org/r/61464/#review58998

Comment 31

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/61466/#review58978

::: dom/media/AccurateSeekTask.cpp:366
(Diff revision 1)
> -  if (audio->mDiscontinuity) {
> -    mDropAudioUntilNextDiscontinuity = false;
> +  if (mFirstAudioSample) {
> +    mFirstAudioSample = false;
> +    MOZ_ASSERT(audio->mDiscontinuity);

The mFirstAudioSample is only used here to place the assertion. How about we make this flag as a mermber of the lambda in the AccurateSeekTask::SetCallbacks() and do this check and assertion in the lambda funtion? Will it be better in this way? At least, by this way, we don't need to have mFirstAudioSample as a member of AccurateSeekTask.

Same to the video case.

Comment 32

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766631 [details]
Bug 1282658. Part 15 - optimize checking of seek complete.

https://reviewboard.mozilla.org/r/61468/#review58982

::: dom/media/AccurateSeekTask.cpp:285
(Diff revision 1)
> -void
> +bool
>  AccurateSeekTask::CheckIfSeekComplete()
>  {
>    AssertOwnerThread();
> -
> -  if (!mDoneVideoSeeking) {
> -    // We haven't reached the target. Ensure we have requested another sample.
> -    EnsureVideoDecodeTaskQueued();
> -  }
> -
> -  if (!mDoneAudioSeeking) {
> -    // We haven't reached the target. Ensure we have requested another sample.
> -    EnsureAudioDecodeTaskQueued();
> -  }
> -
> -  SAMPLE_LOG("CheckIfSeekComplete() doneAudio=%d doneVideo=%d",
> -             mDoneAudioSeeking, mDoneVideoSeeking);
> -
>    if (mDoneAudioSeeking && mDoneVideoSeeking) {
>      Resolve(__func__); // Call to MDSM::SeekCompleted();
> +    return true;
>    }
> +  return false;
>  }

I agree to move "requesting audio/video data" out of this method which makes this method clean, however, I suggest to rename this method to somethig like "ResolvePromiseIfSeekCompleted" since this is the only duty of this method thereafter.

By this way, ResolvePromiseIfSeekCompleted() doen't event need to return a boolean because the boolean value is only used to ask for decoding more audio/video data and for the resason addressed bellow, this boolean is not needed.

::: dom/media/AccurateSeekTask.cpp:370
(Diff revision 1)
> -  CheckIfSeekComplete();
> +  if (!CheckIfSeekComplete() && !mDoneAudioSeeking) {
> +    EnsureAudioDecodeTaskQueued();
> +  }

Then, we don't to check "!CheckIfSeekComplete()" in this if-statement. "!CheckIfSeekComplete() && !mDoneAudioSeeking" has the same true-false table to the "!mDoneAudioSeeking". Same to the video case.

::: dom/media/AccurateSeekTask.cpp:417
(Diff revision 1)
>          // Hit the end of stream. Move mFirstVideoFrameAfterSeek into
>          // mSeekedVideoData so we have something to display after seeking.
>          mSeekedVideoData = mFirstVideoFrameAfterSeek.forget();
>        }
>      }
>      CheckIfSeekComplete();

The method call here is only used to resolve promise is both audio and video seeking are done. Requesting more audio/video data is not the bussiness here.

Comment 33

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766632 [details]
Bug 1282658. Part 16 - remove checks for |mReader->IsWaiting{Audio,Video}Data()|.

https://reviewboard.mozilla.org/r/61470/#review59000

::: dom/media/AccurateSeekTask.cpp:107
(Diff revision 1)
>  AccurateSeekTask::EnsureAudioDecodeTaskQueued()
>  {
>    AssertOwnerThread();
>    MOZ_ASSERT(!mDoneAudioSeeking);
>    MOZ_ASSERT(!mReader->IsRequestingAudioData());
> -
> +  MOZ_ASSERT(!mReader->IsWaitingAudioData());
> -  SAMPLE_LOG("EnsureAudioDecodeTaskQueued status=%s", AudioRequestStatus());
> -
> -  if (mReader->IsWaitingAudioData()) {
> -    return;
> -  }
> -
>    mReader->RequestAudioData();
>  }

Should we rename this method to just "RequestAudioData"? since this is the only duty of this method now. Same to the video case.

Comment 34

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

https://reviewboard.mozilla.org/r/61446/#review58758

> We need to keep the "{audio,video} queue finished" information. So that we can notify the MDSM.

Good catch! I guess I accidentally deleted the line | mIsAudioQueueFinished = true;|.

Comment 35

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

https://reviewboard.mozilla.org/r/61452/#review58968

> Then, mNeedToStopPrerollingAudio and mNeedToStopPrerollingVideo are not used anymore, they are not used in the NextFrameSeekTask. We could just remove them from the SeekTask.h.

Will fix it in a new bug. This bug will focus on changes in AccurateSeekTask.

Comment 36

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/61438/#review58782

::: dom/media/AccurateSeekTask.cpp:127
(Diff revision 1)
> -  RequestVideoData();
> -  return NS_OK;
>  }
>  
>  const char*
>  AccurateSeekTask::AudioRequestStatus()

We have {Audio,Video}RequestStatus() in MDSM, AccurateSeekTaks and NextFrameSeekTask; and they are all the same. I think we could move {Audio,Video}RequestStatus() into MediaDecoderReaderWrapper. This is not belong to the scope of this bug, just make a note here and maybe I can handle it later.

Comment 37

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

https://reviewboard.mozilla.org/r/61466/#review58978

> The mFirstAudioSample is only used here to place the assertion. How about we make this flag as a mermber of the lambda in the AccurateSeekTask::SetCallbacks() and do this check and assertion in the lambda funtion? Will it be better in this way? At least, by this way, we don't need to have mFirstAudioSample as a member of AccurateSeekTask.
> 
> Same to the video case.

Good idea! I will try that.

Comment 38

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

https://reviewboard.mozilla.org/r/61466/#review58978

> Good idea! I will try that.

Well... The lambda is copied during the notification of MediaEventSource so changes to the flag will not change the original copy. We will hit the assertion because firstAudioSample is always true.

Comment 39

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

https://reviewboard.mozilla.org/r/61468/#review58982

> I agree to move "requesting audio/video data" out of this method which makes this method clean, however, I suggest to rename this method to somethig like "ResolvePromiseIfSeekCompleted" since this is the only duty of this method thereafter.
> 
> By this way, ResolvePromiseIfSeekCompleted() doen't event need to return a boolean because the boolean value is only used to ask for decoding more audio/video data and for the resason addressed bellow, this boolean is not needed.

Will call it "MaybeFinishSeek()" and return type will be void.

> Then, we don't to check "!CheckIfSeekComplete()" in this if-statement. "!CheckIfSeekComplete() && !mDoneAudioSeeking" has the same true-false table to the "!mDoneAudioSeeking". Same to the video case.

The code can be structed as:

if (!mDoneAudioSeeking) {
  EnsureAudioDecodeTaskQueued();
  return; // no need to call CheckIfSeekComplete() because seek is definitely not completed.
}
CheckIfSeekComplete();

So we don't need the return value of CheckIfSeekComplete().

> The method call here is only used to resolve promise is both audio and video seeking are done. Requesting more audio/video data is not the bussiness here.

I don't get it. Can you elaborate it?

Comment 40

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/61468/#review58982

> I don't get it. Can you elaborate it?

Sorry, my fault. I was just responding to the suggestion of "ResolvePromiseIfSeekCompleted" (or "MaybeFinishSeek()") above. This is not an issue.

Comment 41

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/61466/#review58978

> Well... The lambda is copied during the notification of MediaEventSource so changes to the flag will not change the original copy. We will hit the assertion because firstAudioSample is always true.

QQ..., r+ then.

Comment 42

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766630 [details]
Bug 1282658. Part 14 - remove mDrop{Audio,Video}UntilNextDiscontinuity for MediaData::mDiscontinuity is guaranteed to be true for the 1st sample after seeking.

https://reviewboard.mozilla.org/r/61466/#review59032

Comment 43

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

https://reviewboard.mozilla.org/r/61470/#review59000

> Should we rename this method to just "RequestAudioData"? since this is the only duty of this method now. Same to the video case.

Good idea!

Comment 44

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

Attachment: Bug 1282658. Part 17 - rename and remove some functions.

Review commit: https://reviewboard.mozilla.org/r/62264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62264/

Comment 45

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

Comment on attachment 8766617 [details]
Bug 1282658. Part 1 - remove some null-checks. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61440/diff/1-2/

Comment 46

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

Comment on attachment 8766618 [details]
Bug 1282658. Part 2 - remove unnecessary checks for Exists().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61442/diff/1-2/

Comment 47

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

Comment on attachment 8766619 [details]
Bug 1282658. Part 3 - rename some functions. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61444/diff/1-2/

Comment 48

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

Comment on attachment 8766620 [details]
Bug 1282658. Part 4 - simplify logic of checking whether if audio/video seeking is completed. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61446/diff/1-2/

Comment 49

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

Comment on attachment 8766621 [details]
Bug 1282658. Part 5 - remove unused members. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61448/diff/1-2/

Comment 50

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

Comment on attachment 8766622 [details]
Bug 1282658. Part 6 - remove unnecessary checks for |mSeekRequest.Exists()|. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61450/diff/1-2/

Comment 51

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

Comment on attachment 8766623 [details]
Bug 1282658. Part 7 - remove setting mNeedToStopPrerolling{Audio,Video} per discussion in https://reviewboard.mozilla.org/r/43689/#comment54421.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61452/diff/1-2/

Comment 52

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

Comment on attachment 8766624 [details]
Bug 1282658. Part 8 - remove asserting |mReader->IsWaitForDataSupported()| for mReader->WaitForData() will crash if the method has no override.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61454/diff/1-2/

Comment 53

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

Comment on attachment 8766625 [details]
Bug 1282658. Part 9 - return void for Ensure{Audio,Video}DecodeTaskQueued() for they always return NS_OK. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61456/diff/1-2/

Comment 54

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

Comment on attachment 8766626 [details]
Bug 1282658. Part 10 - merge Request{Audio,Video}Data() into Ensure{Audio,Video}DecodeTaskQueued() for the logs are not quite useful.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61458/diff/1-2/

Comment 55

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

Comment on attachment 8766627 [details]
Bug 1282658. Part 11 - merge OnAudioNotDecoded() and OnVideoNotDecoded().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61460/diff/1-2/

Comment 56

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

Comment on attachment 8766628 [details]
Bug 1282658. Part 12 - extract common code about adjusting fast seek.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61462/diff/1-2/

Comment 57

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

Comment on attachment 8766629 [details]
Bug 1282658. Part 13 - replace use of int64_t with media::TimeUnit.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61464/diff/1-2/

Comment 58

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

Comment on attachment 8766630 [details]
Bug 1282658. Part 14 - remove mDrop{Audio,Video}UntilNextDiscontinuity for MediaData::mDiscontinuity is guaranteed to be true for the 1st sample after seeking.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61466/diff/1-2/

Comment 59

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

Comment on attachment 8766631 [details]
Bug 1282658. Part 15 - optimize checking of seek complete.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61468/diff/1-2/

Comment 60

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

Comment on attachment 8766632 [details]
Bug 1282658. Part 16 - remove checks for |mReader->IsWaiting{Audio,Video}Data()|.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61470/diff/1-2/

Comment 61

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766620 [details]
Bug 1282658. Part 4 - simplify logic of checking whether if audio/video seeking is completed. .

https://reviewboard.mozilla.org/r/61446/#review59050

Comment 62

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766631 [details]
Bug 1282658. Part 15 - optimize checking of seek complete.

https://reviewboard.mozilla.org/r/61468/#review59052

Comment 63

[Tzuhao Kuo [:kaku]]

Comment on attachment 8767883 [details]
Bug 1282658. Part 17 - rename and remove some functions.

https://reviewboard.mozilla.org/r/62264/#review59054

Comment 64

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

Thanks for the review!

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dbc6fc64432
Part 1 - remove some null-checks. r=kaku.
https://hg.mozilla.org/integration/autoland/rev/badd48f1ff4a
Part 2 - remove unnecessary checks for Exists(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/156ad7d5fdac
Part 3 - rename some functions. r=kaku.
https://hg.mozilla.org/integration/autoland/rev/05bcd2db0e70
Part 4 - simplify logic of checking whether if audio/video seeking is completed. r=kaku.
https://hg.mozilla.org/integration/autoland/rev/18a1d4f64db9
Part 5 - remove unused members. r=kaku.
https://hg.mozilla.org/integration/autoland/rev/59f502a9f190
Part 6 - remove unnecessary checks for |mSeekRequest.Exists()|. r=kaku.
https://hg.mozilla.org/integration/autoland/rev/cea92d10be95
Part 7 - remove setting mNeedToStopPrerolling{Audio,Video} per discussion in https://reviewboard.mozilla.org/r/43689/#comment54421. r=kaku
https://hg.mozilla.org/integration/autoland/rev/c0910b20aefc
Part 8 - remove asserting |mReader->IsWaitForDataSupported()| for mReader->WaitForData() will crash if the method has no override. r=kaku
https://hg.mozilla.org/integration/autoland/rev/1668f3d274b7
Part 9 - return void for Ensure{Audio,Video}DecodeTaskQueued() for they always return NS_OK. r=kaku.
https://hg.mozilla.org/integration/autoland/rev/a0325c3082b8
Part 10 - merge Request{Audio,Video}Data() into Ensure{Audio,Video}DecodeTaskQueued() for the logs are not quite useful. r=kaku
https://hg.mozilla.org/integration/autoland/rev/111114fbbe27
Part 11 - merge OnAudioNotDecoded() and OnVideoNotDecoded(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/eb18fb6afd68
Part 12 - extract common code about adjusting fast seek. r=kaku
https://hg.mozilla.org/integration/autoland/rev/d7f5babfb534
Part 13 - replace use of int64_t with media::TimeUnit. r=kaku
https://hg.mozilla.org/integration/autoland/rev/4c079c504dfe
Part 14 - remove mDrop{Audio,Video}UntilNextDiscontinuity for MediaData::mDiscontinuity is guaranteed to be true for the 1st sample after seeking. r=kaku
https://hg.mozilla.org/integration/autoland/rev/44ed26a7269a
Part 15 - optimize checking of seek complete. r=kaku
https://hg.mozilla.org/integration/autoland/rev/0468b7cedfcf
Part 16 - remove checks for |mReader->IsWaiting{Audio,Video}Data()|. r=kaku
https://hg.mozilla.org/integration/autoland/rev/28c44b72633b
Part 17 - rename and remove some functions. r=kaku

Comment 66

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4dbc6fc64432
https://hg.mozilla.org/mozilla-central/rev/badd48f1ff4a
https://hg.mozilla.org/mozilla-central/rev/156ad7d5fdac
https://hg.mozilla.org/mozilla-central/rev/05bcd2db0e70
https://hg.mozilla.org/mozilla-central/rev/18a1d4f64db9
https://hg.mozilla.org/mozilla-central/rev/59f502a9f190
https://hg.mozilla.org/mozilla-central/rev/cea92d10be95
https://hg.mozilla.org/mozilla-central/rev/c0910b20aefc
https://hg.mozilla.org/mozilla-central/rev/1668f3d274b7
https://hg.mozilla.org/mozilla-central/rev/a0325c3082b8
https://hg.mozilla.org/mozilla-central/rev/111114fbbe27
https://hg.mozilla.org/mozilla-central/rev/eb18fb6afd68
https://hg.mozilla.org/mozilla-central/rev/d7f5babfb534
https://hg.mozilla.org/mozilla-central/rev/4c079c504dfe
https://hg.mozilla.org/mozilla-central/rev/44ed26a7269a
https://hg.mozilla.org/mozilla-central/rev/0468b7cedfcf
https://hg.mozilla.org/mozilla-central/rev/28c44b72633b