Closed Bug 1261020 Opened 4 years ago Closed 4 years ago

Separate the logic of seek operation out of MDSM.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

Separate the logic of seek operation out of MDSM into a isolated class (SeekTask). By this way, the MDSM could be simplified and the isolated class could further be extended to implement HTMLMediaElement::SeekToNextFrame() (Bug 1235301).
Attachment #8737053 - Flags: review?(jwwang)
Comment on attachment 8737053 [details]
MozReview Request: Bug 1261020 - part 2 - modify MDSM to adopt SeekTask; r=jwwang

https://reviewboard.mozilla.org/r/43689/#review40253

::: dom/media/MediaDecoderStateMachine.h:1047
(Diff revision 1)
> -  MozPromiseRequestHolder<MediaDecoderReader::SeekPromise> mSeekRequest;
> -
>    // We record the playback position before we seek in order to
>    // determine where the seek terminated relative to the playback position
>    // we were at before the seek.
>    int64_t mCurrentTimeBeforeSeek;

This member is not needed by MDSM anymore.

::: dom/media/MediaDecoderStateMachine.cpp:1141
(Diff revision 1)
>        // XXXbholley - Nobody is listening to this promise. Do we need to pass it
>        // back to MediaDecoder when we come out of dormant?
>        RefPtr<MediaDecoder::SeekPromise> unused = mQueuedSeek.mPromise.Ensure(__func__);
>      }
> -    mCurrentSeek.RejectIfExists(__func__);
> +
> +    if (mSeekTask) { mSeekTask->Discard(); }

Nullify mSeekTask when exiting SEEKING.

::: dom/media/MediaDecoderStateMachine.cpp:1456
(Diff revision 1)
>  void
>  MediaDecoderStateMachine::InitiateSeek(SeekJob aSeekJob)
>  {
>    MOZ_ASSERT(OnTaskQueue());
>  
> -  mCurrentSeek.RejectIfExists(__func__);
> +  if (mSeekTask) { mSeekTask->Discard(); }

style: always break lines for if statement.

::: dom/media/MediaDecoderStateMachine.cpp:1459
(Diff revision 1)
>    MOZ_ASSERT(OnTaskQueue());
>  
> -  mCurrentSeek.RejectIfExists(__func__);
> -  mCurrentSeek = Move(aSeekJob);
> +  if (mSeekTask) { mSeekTask->Discard(); }
> +  mSeekTaskRequest.DisconnectIfExists();
> +  mSeekTask = SeekTask::CreateSeekTask(mDecoderID, OwnerThread(), mReader.get(),
> +                                       aSeekJob, HasAudio(), HasVideo(),

Just pass mInfo without passing HasAudio(), HasVideo() and mInfo.mAudio.mRate separately.

::: dom/media/MediaDecoderStateMachine.cpp:1462
(Diff revision 1)
> -  mCurrentSeek = Move(aSeekJob);
> +  mSeekTaskRequest.DisconnectIfExists();
> +  mSeekTask = SeekTask::CreateSeekTask(mDecoderID, OwnerThread(), mReader.get(),
> +                                       aSeekJob, HasAudio(), HasVideo(),
> +                                       mInfo.mAudio.mRate, StartTime());
>  
> -  // Bound the seek time to be inside the media range.
> +  mSeekTask->Init(Duration().ToMicroseconds(), GetMediaTime());

I don't like 2-phase construction. Pass all the parameters to the creator.

::: dom/media/MediaDecoderStateMachine.cpp:1476
(Diff revision 1)
>  
>    // Reset our state machine and decoding pipeline before seeking.
> -  Reset();
> +  if (mSeekTask->NeedToResetMDSM()) { Reset(); }
>  
>    // Do the seek.
> -  RefPtr<MediaDecoderStateMachine> self = this;
> +  mSeekTaskRequest.Begin(InvokeAsync(OwnerThread(), mSeekTask.get(), __func__,

Just call |mSeekTask->Seek()| which should dispatch tasks if necessary.

::: dom/media/MediaDecoderStateMachine.cpp:1485
(Diff revision 1)
> -                                 &MediaDecoderReader::Seek, seekTarget,
> -                                 Duration().ToMicroseconds())
> -    ->Then(OwnerThread(), __func__,
> -           [self] (media::TimeUnit) -> void {
> -             self->mSeekRequest.Complete();
> -             // We must decode the first samples of active streams, so we can determine
> +           &MediaDecoderStateMachine::OnSeekTaskRejected));
> +}
> +
> +void
> +MediaDecoderStateMachine::OnSeekTaskResolved(SeekTaskResolveValue aValue)
> +{

MOZ_ASSERT(OnTaskQueue());

::: dom/media/MediaDecoderStateMachine.cpp:1500
(Diff revision 1)
> +    Push(aValue.mSeekedVideoData.get(), MediaData::VIDEO_DATA);
> +    mDecodedVideoEndTime =
> +      std::max(aValue.mSeekedVideoData->GetEndTime(), mDecodedVideoEndTime);
> +  }
> +
> +  if (aValue.mIsAudioQueueFinished) {

Need to handle audio/video prerolling.

::: dom/media/MediaDecoderStateMachine.cpp:1508
(Diff revision 1)
> +
> +  if (aValue.mIsVideoQueueFinished) {
> +    VideoQueue().Finish();
> +  }
> +
> +  SeekCompleted();

When seek is finished, we should nullify mSeekTask because it should never be used outside seeking, right?

::: dom/media/MediaDecoderStateMachine.cpp:1513
(Diff revision 1)
> +  SeekCompleted();
> +}
> +
> +void
> +MediaDecoderStateMachine::OnSeekTaskRejected(SeekTaskRejectValue aValue)
> +{

MOZ_ASSERT(OnTaskQueue());

::: dom/media/MediaDecoderStateMachine.cpp:1524
(Diff revision 1)
> +
> +  if (aValue.mIsVideoQueueFinished) {
> +    VideoQueue().Finish();
> +  }
> +
> +  if (aValue.mIsDecodeError) {

How could SeekTask be rejected without any error?
Blocks: 1235301
Priority: -- → P2
Blocks: 1253184
Assignee: nobody → tkuo
Comment on attachment 8737052 [details]
MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r=jwwang

https://reviewboard.mozilla.org/r/43687/#review40255

::: dom/media/MediaDecoderStateMachine.h
(Diff revision 1)
>    // The decoder monitor must be held.
>    void EnqueueLoadedMetadataEvent();
>  
>    void EnqueueFirstFrameLoadedEvent();
>  
> -  struct SeekJob {

Use "hg cp" to move SeekJob to its own header and reserve the history.

::: dom/media/MediaDecoderStateMachine.h:519
(Diff revision 1)
> -    MozPromiseHolder<MediaDecoder::SeekPromise> mPromise;
> -  };
> -
>    // Clears any previous seeking state and initiates a new see on the decoder.
>    // The decoder monitor must be held.
> -  void InitiateSeek(SeekJob aSeekJob);
> +  void InitiateSeek(media::SeekJob aSeekJob);

It is less verbose to put SeekJob under the namespace mozilla.

::: dom/media/SeekTask.h:47
(Diff revision 1)
> +  SeekTaskReturnValueBase(bool aIsAudioQueueFinished, bool aIsVideoQueueFinished);
> +  bool mIsAudioQueueFinished;
> +  bool mIsVideoQueueFinished;
> +};
> +
> +struct SeekTaskResolveValue : public SeekTaskReturnValueBase{

Don't use inheritance just for sharing memebers only.

::: dom/media/SeekTask.h:48
(Diff revision 1)
> +  bool mIsAudioQueueFinished;
> +  bool mIsVideoQueueFinished;
> +};
> +
> +struct SeekTaskResolveValue : public SeekTaskReturnValueBase{
> +  SeekTaskResolveValue(MediaData* aAudioData, MediaData* aVideoData,

The constructor is used to enforce invariants of the object. In your case, you should just assign each member one by one without calling the constructor.

::: dom/media/SeekTask.cpp:129
(Diff revision 1)
> +}
> +
> +SeekTask::SeekTask(const void* aDecoderID,
> +                   AbstractThread* aThread,
> +                   MediaDecoderReader* aReader,
> +                   SeekJob& aSeekJob,

Use rvalue reference since you will move it anyway.

::: dom/media/SeekTask.cpp:131
(Diff revision 1)
> +SeekTask::SeekTask(const void* aDecoderID,
> +                   AbstractThread* aThread,
> +                   MediaDecoderReader* aReader,
> +                   SeekJob& aSeekJob,
> +                   bool aHasAudio, bool aHasVideo,
> +                   uint32_t aAudioRate, uint64_t aStartTime)

aStartTime not used at all.

::: dom/media/SeekTask.cpp:132
(Diff revision 1)
> +                   AbstractThread* aThread,
> +                   MediaDecoderReader* aReader,
> +                   SeekJob& aSeekJob,
> +                   bool aHasAudio, bool aHasVideo,
> +                   uint32_t aAudioRate, uint64_t aStartTime)
> +: mDecoderID(aDecoderID)

2-spaces indentation.

::: dom/media/SeekTask.cpp:254
(Diff revision 1)
> +}
> +
> +RefPtr<SeekTask::SeekTaskPromise>
> +SeekTask::Seek(int64_t aDuration)
> +{
> +  // This SeekTask might has been discarded before this method been called.

How is this possible?

::: dom/media/SeekTask.cpp:367
(Diff revision 1)
> +void
> +SeekTask::RequestVideoData()
> +{
> +  AssertOwnerThread();
> +
> +  //These two variables are not used in the SEEKING state.

Delete unused lines.

::: dom/media/SeekTask.cpp:449
(Diff revision 1)
> +                                       duration.value(),
> +                                       frames,
> +                                       Move(audioData),
> +                                       channels,
> +                                       audio->mRate));
> +  MOZ_ASSERT(mSeekedAudioData == nullptr, "Should be the 1st sample after seeking");

Just say |MOZ_ASSERT(mSeekedAudioData, ...)|.

::: dom/media/SeekTask.cpp:502
(Diff revision 1)
> +  SAMPLE_LOG("IsAudioSeekComplete() curTarVal=%d mAudDis=%d aqFin=%d aqSz=%d",
> +      mSeekJob.Exists(), mDropAudioUntilNextDiscontinuity, mIsAudioQueueFinished, !!mSeekedAudioData);
> +  return
> +    !HasAudio() ||
> +    (Exists() && !mDropAudioUntilNextDiscontinuity &&
> +     (mIsAudioQueueFinished || mSeekedAudioData != nullptr));

Just say "|| mSeekedAudioData" without explicitly comparing it with nullptr.

::: dom/media/SeekTask.cpp:580
(Diff revision 1)
> +{
> +  AssertOwnerThread();
> +  RefPtr<MediaData> audio(aAudioSample);
> +  MOZ_ASSERT(audio);
> +  mAudioDataRequest.Complete();
> +  aAudioSample->AdjustForStartTime(mStartTime);

You will need to rebase on bug 1250054.

::: dom/media/SeekTask.cpp:643
(Diff revision 1)
> +    mIsDecodeError = true;
> +    RejectIfExist(__func__);
> +    return;
> +  }
> +
> +  // MediaDecoderReader::WAITING_FOR_DATA and MediaDecoderReader::CANCELED are

Why not?
Attachment #8737052 - Flags: review?(jwwang)
https://reviewboard.mozilla.org/r/43687/#review40271

Ok for the other comments.

::: dom/media/SeekTask.cpp:254
(Diff revision 1)
> +}
> +
> +RefPtr<SeekTask::SeekTaskPromise>
> +SeekTask::Seek(int64_t aDuration)
> +{
> +  // This SeekTask might has been discarded before this method been called.

This is my fault. I tried to call SeekTask::Seek() asynchronously in the MDSM::InitiateSeek() whihc is a bug.

In next patch, I will call SeekTask::Seek() synchronously in the MDSM::InitiateSeek() and delete this part.

::: dom/media/SeekTask.cpp:643
(Diff revision 1)
> +    mIsDecodeError = true;
> +    RejectIfExist(__func__);
> +    return;
> +  }
> +
> +  // MediaDecoderReader::WAITING_FOR_DATA and MediaDecoderReader::CANCELED are

Because, for both conditions, the current code base calls DispatchDecodeTasksIfNeeded() which then calls NeedToDecodeAudio()/NeedToDecodeVideo().

NeedToDecodeAudio()/NeedToDecodeVideo() always returns FALSE if mState == DECODER_STATE_SEEKING. (This behavior starts from Bug 1252762)

So, I think the codes in these conditions actually do nothing if mState == DECODER_STATE_SEEKING, rigiht?
https://reviewboard.mozilla.org/r/43687/#review40255

> aStartTime not used at all.

Do u mean after rebasing onto rebase on bug 1250054?

> Just say |MOZ_ASSERT(mSeekedAudioData, ...)|.

Should it be |MOZ_ASSERT(!mSeekedAudioData, ...)| ?
https://reviewboard.mozilla.org/r/43687/#review40255

> Do u mean after rebasing onto rebase on bug 1250054?

Sorry my fault. It is assigned to mStartTime.

> Should it be |MOZ_ASSERT(!mSeekedAudioData, ...)| ?

Right!
https://reviewboard.mozilla.org/r/43687/#review40255

> How is this possible?

This is my fault. I tried to call SeekTask::Seek() asynchronously in the MDSM::InitiateSeek() whihc is a bug.

In next patch, I will call SeekTask::Seek() synchronously in the MDSM::InitiateSeek() and delete this part.

> Why not?

*I should use reply instead of review.

Because, for both conditions, the current code base calls DispatchDecodeTasksIfNeeded() which then calls NeedToDecodeAudio()/NeedToDecodeVideo().

NeedToDecodeAudio()/NeedToDecodeVideo() always returns FALSE if mState == DECODER_STATE_SEEKING. (This behavior starts from Bug 1252762)

So, I think the codes in these conditions actually do nothing if mState == DECODER_STATE_SEEKING, rigiht? or this is a mistak of Bug 1252762 ?
https://reviewboard.mozilla.org/r/43687/#review40255

> *I should use reply instead of review.
> 
> Because, for both conditions, the current code base calls DispatchDecodeTasksIfNeeded() which then calls NeedToDecodeAudio()/NeedToDecodeVideo().
> 
> NeedToDecodeAudio()/NeedToDecodeVideo() always returns FALSE if mState == DECODER_STATE_SEEKING. (This behavior starts from Bug 1252762)
> 
> So, I think the codes in these conditions actually do nothing if mState == DECODER_STATE_SEEKING, rigiht? or this is a mistak of Bug 1252762 ?

Yes, it sounds like a regression from bug 1252762. MDSM should continue decoding after wait promise is resolved during seeking. Please file a bug to fix it.
https://reviewboard.mozilla.org/r/43687/#review40255

> Yes, it sounds like a regression from bug 1252762. MDSM should continue decoding after wait promise is resolved during seeking. Please file a bug to fix it.

How about let's fix it in THIS bug? Seeking operation will never go into the original code path anymore.
https://reviewboard.mozilla.org/r/43687/#review40255

> How about let's fix it in THIS bug? Seeking operation will never go into the original code path anymore.

I would prefer to fix it in a new bug which we might need to uplift to beta/aurora.
Depends on: 1261312
Depends on: 1250054
https://reviewboard.mozilla.org/r/43687/#review40255

> I would prefer to fix it in a new bug which we might need to uplift to beta/aurora.

Done. Here you are. Bug 1261312.
https://reviewboard.mozilla.org/r/43687/#review40255

> Done. Here you are. Bug 1261312.

I will then modify this part based on Bug 1261312.
Comment on attachment 8737052 [details]
MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43687/diff/1-2/
Attachment #8737052 - Flags: review?(jwwang)
Attachment #8737053 - Flags: review?(jwwang)
Comment on attachment 8737053 [details]
MozReview Request: Bug 1261020 - part 2 - modify MDSM to adopt SeekTask; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43689/diff/1-2/
Comment on attachment 8737053 [details]
MozReview Request: Bug 1261020 - part 2 - modify MDSM to adopt SeekTask; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43689/diff/2-3/
Attachment #8737053 - Flags: review?(jwwang)
Comment on attachment 8737053 [details]
MozReview Request: Bug 1261020 - part 2 - modify MDSM to adopt SeekTask; r=jwwang

https://reviewboard.mozilla.org/r/43689/#review41865

::: dom/media/MediaDecoderStateMachine.cpp:1501
(Diff revision 3)
> -    mReaderWrapper->Seek(mCurrentSeek.mTarget, Duration())
> -    ->Then(OwnerThread(), __func__,
> -           [self] (media::TimeUnit) -> void {
> -             self->mSeekRequest.Complete();
> -             // We must decode the first samples of active streams, so we can determine
> -             // the new stream time. So dispatch tasks to do that.
> +           &MediaDecoderStateMachine::OnSeekTaskResolved,
> +           &MediaDecoderStateMachine::OnSeekTaskRejected));
> +}
> +
> +void
> +MediaDecoderStateMachine::OnSeekTaskResolved(SeekTaskResolveValue aValue)

Assert |mState == DECODER_STATE_SEEKING| in this function.

::: dom/media/MediaDecoderStateMachine.cpp:1536
(Diff revision 3)
> +  }
> +
> +  if (aValue.mNeedToStopPrerollingVideo) {
> +    StopPrerollingVideo();
> +  }
> +

Why do we need to stop prerolling in above statements?

::: dom/media/MediaDecoderStateMachine.cpp:1543
(Diff revision 3)
> +
> +  mSeekTask = nullptr;
> +}
> +
> +void
> +MediaDecoderStateMachine::OnSeekTaskRejected(SeekTaskRejectValue aValue)

Ditto.
Comment on attachment 8737052 [details]
MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r=jwwang

https://reviewboard.mozilla.org/r/43687/#review41867

::: dom/media/SeekJob.h:1
(Diff revision 2)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Use "hg cp dom/media/MediaDecoderStateMachine.h dom/media/SeekJob.h" and delete unwanted lines for SeekJob.h to preserve history.

::: dom/media/SeekTask.h:50
(Diff revision 2)
> +  static const bool IsExclusive = true;
> +
> +  using SeekTaskPromise =
> +    MozPromise<SeekTaskResolveValue, SeekTaskRejectValue, IsExclusive>;
> +
> +  static SeekTask* CreateSeekTask(const void* aDecoderID,

Return already_AddRefed<> to avoid resource leaks.

::: dom/media/SeekTask.cpp:49
(Diff revision 2)
> +                         const MediaInfo& aInfo,
> +                         const media::TimeUnit& aDuration,
> +                         int64_t aCurrentMediaTime)
> +{
> +  return new SeekTask(aDecoderID, aThread, aReader, aReaderWrapper,
> +                      Forward<SeekJob>(aSeekJob), aInfo, aDuration,

Use |Move(aSeekJob)|.

::: dom/media/SeekTask.cpp:93
(Diff revision 2)
> +  mDropVideoUntilNextDiscontinuity = HasVideo();
> +}
> +
> +SeekTask::~SeekTask()
> +{
> +  Discard();

We should assert this SeekTask is either completed or discarded by the client code before destruction. Implicit cleanup doesn't work for MozPromise.
Attachment #8737052 - Flags: review?(jwwang)
https://reviewboard.mozilla.org/r/43689/#review41865

> Why do we need to stop prerolling in above statements?

I try to preserve the behavior of stopping prerolling(https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#801) in the  MediaDecoderStateMachine::OnNotDecoded() into the SeekTask::OnAudio/VideoNotDecoded().

So, in the SeekTask::OnAudio/VideoNotDecoded(), if MediaDecoderReader::WAITING_FOR_DATA happened, I will then send a signal back to MDSM to stop prerolling.

Does it make sense?
https://reviewboard.mozilla.org/r/43689/#review41865

> I try to preserve the behavior of stopping prerolling(https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#801) in the  MediaDecoderStateMachine::OnNotDecoded() into the SeekTask::OnAudio/VideoNotDecoded().
> 
> So, in the SeekTask::OnAudio/VideoNotDecoded(), if MediaDecoderReader::WAITING_FOR_DATA happened, I will then send a signal back to MDSM to stop prerolling.
> 
> Does it make sense?

In fact, prerolling is something about DECODER_STATE_DECODING which SeekTask should not know about. We can fix it in next bugs.
https://reviewboard.mozilla.org/r/43689/#review41865

> In fact, prerolling is something about DECODER_STATE_DECODING which SeekTask should not know about. We can fix it in next bugs.

Ok, I will track it.
Comment on attachment 8737052 [details]
MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43687/diff/2-3/
Attachment #8737052 - Flags: review?(jwwang)
Attachment #8737053 - Flags: review?(jwwang)
Comment on attachment 8737053 [details]
MozReview Request: Bug 1261020 - part 2 - modify MDSM to adopt SeekTask; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43689/diff/3-4/
Comment on attachment 8737052 [details]
MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r=jwwang

https://reviewboard.mozilla.org/r/43687/#review41971
Attachment #8737052 - Flags: review?(jwwang) → review+
Comment on attachment 8737053 [details]
MozReview Request: Bug 1261020 - part 2 - modify MDSM to adopt SeekTask; r=jwwang

https://reviewboard.mozilla.org/r/43689/#review41973
Attachment #8737053 - Flags: review?(jwwang) → review+
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b80e00dc74a8

Looks like there are regressions.

 7 INFO TEST-UNEXPECTED-FAIL | dom/media/mediasource/test/test_BufferedSeek_mp4.html | Video currentTime at target - got 1.253877, expected 1.3

87 INFO TEST-UNEXPECTED-FAIL | dom/media/mediasource/test/test_SeekNoData_mp4.html | Time >= 5
165 INFO TEST-UNEXPECTED-FAIL | dom/media/mediasource/test/test_SeekTwice_mp4.html | Time >= 6

383 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_fragment_play.html | big.wav#t=7,15 fragment test: seeked currentTime is 6.965986 != 7

401 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_fragment_play.html | big.wav#t=5 fragment test: seeked currentTime is 4.94585 != 5

410 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_fragment_play.html | big.wav#t=5.5 fragment test: seeked currentTime is 5.433469 != 5.5
Comment on attachment 8737052 [details]
MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43687/diff/3-4/
Attachment #8737052 - Attachment description: MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r?jwwang → MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r=jwwang
Attachment #8737053 - Attachment description: MozReview Request: Bug 1261020 - part 2 - modify MDSM to adopt SeekTask; r?jwwang → MozReview Request: Bug 1261020 - part 2 - modify MDSM to adopt SeekTask; r=jwwang
Comment on attachment 8737053 [details]
MozReview Request: Bug 1261020 - part 2 - modify MDSM to adopt SeekTask; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43689/diff/4-5/
(In reply to Tzuhao Kuo [:kaku] from comment #28)
> Comment on attachment 8737052 [details]
> MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r=jwwang
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/43687/diff/3-4/

JW, I need your review on this patch again. 
This new patch solve the regression I mentioned at comment 27.
The root cause is at SeekTask::DropAudioUpToSeekTarget(), line 382.
Flags: needinfo?(jwwang)
Try looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a4d96da5078&selectedJob=19339179
Thanks for the review!
Flags: needinfo?(jwwang)
Keywords: checkin-needed
Blocks: 1264171
Hi, this has problems to apply:

patching file dom/media/SeekJob.h
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file dom/media/SeekJob.h.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

could you take a look, thanks!
Flags: needinfo?(tkuo)
Comment on attachment 8737053 [details]
MozReview Request: Bug 1261020 - part 2 - modify MDSM to adopt SeekTask; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43689/diff/5-6/
Attachment #8737052 - Attachment is obsolete: true
Flags: needinfo?(tkuo)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #33)
> Hi, this has problems to apply:
> 
> patching file dom/media/SeekJob.h
> Hunk #1 FAILED at 0
> 1 out of 1 hunks FAILED -- saving rejects to file dom/media/SeekJob.h.rej
> patch failed to apply
> abort: fix up the working directory and run hg transplant --continue
> 
> could you take a look, thanks!

I have rebased to the mozilla-inbound, it should work now, thanks!
Comment on attachment 8741259 [details]
MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r=jwwang

carry r+.
Attachment #8741259 - Flags: review?(jwwang) → review+
https://hg.mozilla.org/mozilla-central/rev/a6bda6a1cc47
https://hg.mozilla.org/mozilla-central/rev/ee21e163344b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.