Closed
Bug 1261020
Opened 8 years ago
Closed 8 years ago
Separate the logic of seek operation out of MDSM.
Categories
(Core :: Audio/Video: Playback, defect, P2)
Core
Audio/Video: Playback
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).
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43687/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43687/
Attachment #8737052 -
Flags: review?(jwwang)
Attachment #8737053 -
Flags: review?(jwwang)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43689/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43689/
Updated•8 years ago
|
Attachment #8737053 -
Flags: review?(jwwang)
Comment 3•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → tkuo
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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?
Assignee | ||
Comment 6•8 years ago
|
||
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, ...)| ?
Comment 7•8 years ago
|
||
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!
Assignee | ||
Comment 8•8 years ago
|
||
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 ?
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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.
Comment 11•8 years ago
|
||
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.
Assignee | ||
Comment 12•8 years ago
|
||
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.
Assignee | ||
Comment 13•8 years ago
|
||
https://reviewboard.mozilla.org/r/43687/#review40255 > Done. Here you are. Bug 1261312. I will then modify this part based on Bug 1261312.
Assignee | ||
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
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/
Assignee | ||
Comment 16•8 years ago
|
||
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/
Updated•8 years ago
|
Attachment #8737053 -
Flags: review?(jwwang)
Comment 17•8 years ago
|
||
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 18•8 years ago
|
||
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)
Assignee | ||
Comment 19•8 years ago
|
||
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?
Comment 21•8 years ago
|
||
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.
Assignee | ||
Comment 22•8 years ago
|
||
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.
Assignee | ||
Comment 23•8 years ago
|
||
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)
Assignee | ||
Comment 24•8 years ago
|
||
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 25•8 years ago
|
||
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 26•8 years ago
|
||
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+
Assignee | ||
Comment 27•8 years ago
|
||
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
Assignee | ||
Comment 28•8 years ago
|
||
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
Assignee | ||
Comment 29•8 years ago
|
||
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/
Assignee | ||
Comment 30•8 years ago
|
||
(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)
Assignee | ||
Comment 32•8 years ago
|
||
Try looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a4d96da5078&selectedJob=19339179 Thanks for the review!
Flags: needinfo?(jwwang)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 33•8 years ago
|
||
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)
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 34•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46339/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46339/
Attachment #8741259 -
Flags: review?(jwwang)
Assignee | ||
Comment 35•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8737052 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(tkuo)
Keywords: checkin-needed
Assignee | ||
Comment 36•8 years ago
|
||
(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!
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8741259 [details] MozReview Request: Bug 1261020 - part 1 - implement SeekTask; r=jwwang carry r+.
Attachment #8741259 -
Flags: review?(jwwang) → review+
Comment 38•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6bda6a1cc47 https://hg.mozilla.org/integration/mozilla-inbound/rev/ee21e163344b
Keywords: checkin-needed
Comment 39•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a6bda6a1cc47 https://hg.mozilla.org/mozilla-central/rev/ee21e163344b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•