For now, there is no "non-realtime video source" on the web platform. I would like to add a "SeekToNextFrame()" method to the HTMLMediaElement. With this method enabled, the video source no longer sticks to realtime clock and the underlying data could be accessed in the "frame" unit.

This is the draft webidl patch. Would like to listen to your feedbacks. BTW, thanks jwwang for discussing with me and providing the draft implementation patch.

To increase the flexibility of SeekToNextFrame, It might be better to have one parameter to get those key frames, like SeekToNextFrame(bool keyFrame). I'm not sure whether it has been discussed or not.

(In reply to Blake Wu [:bwu][:blakewu] from comment #2) > To increase the flexibility of SeekToNextFrame, It might be better to have > one parameter to get those key frames, like SeekToNextFrame(bool keyFrame). > I'm not sure whether it has been discussed or not. Not discussed. Any specific use case?

Comment on attachment 8702459 [details] [diff] [review] [WIP] webidl.patch Review of attachment 8702459 [details] [diff] [review]: ----------------------------------------------------------------- Can you give us a JavaScript sample code to show how we use this API? ::: dom/webidl/HTMLMediaElement.webidl @@ +175,5 @@ > + > +/* > + * The SeekToNextFrame() method aims to make a video be played in an offline > + * rate which means that the video element no longer sticks to real-time clock. > + * This method also lets authors use "frame" as unit to access the underlying Should we keep the consistent of the usage of authors and developers?

(In reply to Tzuhao Kuo [:kaku] from comment #3) > (In reply to Blake Wu [:bwu][:blakewu] from comment #2) > > To increase the flexibility of SeekToNextFrame, It might be better to have > > one parameter to get those key frames, like SeekToNextFrame(bool keyFrame). > > I'm not sure whether it has been discussed or not. > > Not discussed. Any specific use case? User may want to get key frames. Previously I was asked if it is possible to get key frames via media element. User can get key frames and use them to generate a gif file. Another use case is user can easily get key frames to generate a series of thumbnails. Those thumbnails may be used to let user know what is inside in that video. For example, it can be shown on player controller UI when user drags the progress bar, like the youtube controller UI. For video editing, user can show them on the timeline without decoding each frame and pick some decoded frames to show on the timeline.

Comment on attachment 8702459 [details] [diff] [review] [WIP] webidl.patch Review of attachment 8702459 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/HTMLMediaElement.webidl @@ +185,5 @@ > + * @return If successfully sought to next frame, resolve the returned promise. > + * For other cases, reject the returned promise with error message. > + */ > +partial interface HTMLMediaElement { > + Promise<void> SeekToNextFrame(); Is it intended to be a variant of seek/fastSeek or a new playback operation like play/pause? If it is the former, you can fire a 'seeked' event to notify the completion of seek operation.

IIUC, the way to use SeekToNextFrame() is user pauses media element and calls it to get the next frame. User can call it many times if he/she wants to get many frames starting from the paused time. Please correct me if I'm wrong. From the name, SeekToNextFrame, it looks like to introduce another seek, but actually it just gets the next frame. Currently in media elment, it already has two methods to do seek, currentTime(accurate seek) and fastseek (not accurate). It should not be good to introduce another one similar name. Users/developers may get confused and could not easily understand which seek they should use. So it would be better to revamp the seek in current media element. IMHO, it would be easy to use, if we change it as below. 1. Make currentTime readonly and depreciate fastSeek in the future. 2. Propose new method, like seek(seekTime, flags) flags: ACCURATE_SEEK, //seek the frame closest to seekTiime FAST_SEEK_TO_CLOSEST_KEYFRAME, //seek to the keyframe closest to seekTime FAST_SEEK_TO_NEXT_KEYFRAME, //seek to the keyframe next to seekTime FAST_SEEK_TO_PREVIOUS_KEYFRAME,//seek to the keyframe previous to seekTime 3. SeekToNextFrame would sound more senses to me to be called "getNextFrame" since it is used to get the following frame from paused time.

Comment on attachment 8702459 [details] [diff] [review] [WIP] webidl.patch Review of attachment 8702459 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/HTMLMediaElement.webidl @@ +185,5 @@ > + * @return If successfully sought to next frame, resolve the returned promise. > + * For other cases, reject the returned promise with error message. > + */ > +partial interface HTMLMediaElement { > + Promise<void> SeekToNextFrame(); I think it should just be another seek operation and use the existing events.

(In reply to Blake Wu [:bwu][:blakewu] from comment #7) > IIUC, the way to use SeekToNextFrame() is user pauses media element and > calls it to get the next frame. User can call it many times if he/she wants > to get many frames starting from the paused time. Please correct me if I'm > wrong. That sounds right. > From the name, SeekToNextFrame, it looks like to introduce another seek, but > actually it just gets the next frame. > > Currently in media elment, it already has two methods to do seek, > currentTime(accurate seek) and fastseek (not accurate). It should not be > good to introduce another one similar name. Users/developers may get > confused and could not easily understand which seek they should use. So it > would be better to revamp the seek in current media element. On the contrary, I think adding another seek method and reusing the existing events is easier to specify and use. Providing another kind of seek operation but making it behave differently from the others would be confusing. > 1. Make currentTime readonly and depreciate fastSeek in the future. We certainly can't make currentTime readonly without breaking the Web. > 2. Propose new method, like seek(seekTime, flags) > flags: > ACCURATE_SEEK, //seek the frame closest to seekTiime > FAST_SEEK_TO_CLOSEST_KEYFRAME, //seek to the keyframe closest to seekTime > FAST_SEEK_TO_NEXT_KEYFRAME, //seek to the keyframe next to seekTime > FAST_SEEK_TO_PREVIOUS_KEYFRAME,//seek to the keyframe previous to seekTime It's often better to have separate methods than a single method that takes a parameter telling it what to do. It might make sense for seekToNextFrame() to have a time parameter and seek to the next frame after that time, although I don't know if there's a real use-case for that. I don't know of any use-cases for explicitly enumerating keyframes other than the usecases already covered by fastSeek.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8) > Comment on attachment 8702459 [details] [diff] [review] > [WIP] webidl.patch > > Review of attachment 8702459 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/webidl/HTMLMediaElement.webidl > @@ +185,5 @@ > > + * @return If successfully sought to next frame, resolve the returned promise. > > + * For other cases, reject the returned promise with error message. > > + */ > > +partial interface HTMLMediaElement { > > + Promise<void> SeekToNextFrame(); > > I think it should just be another seek operation and use the existing events. Agree with it.

Hi, JW, This WIP patch implements the SeekToNextFrame() as a seek operation. Would like to listen to your thought on the current implementation. Thanks, Kaku

Comment on attachment 8704962 [details] [diff] [review] [WIP] Bug1235301-WIP.patch Review of attachment 8704962 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/HTMLMediaElement.cpp @@ +1432,5 @@ > > +void > +HTMLMediaElement::SeekToNextFrame(ErrorResult& aRv) > +{ > + Seek(-1.0, SeekTarget::NextFrame, aRv); See the comment below. Maybe we can just pass currentTime here. @@ +1506,5 @@ > // the seek completes. > mCurrentPlayRangeStart = -1.0; > } > > + // TODO: not sure what to do in this case...... We should do nothing. If we try to seek to next frame before playback starts, we should end up in the 1st frame, right? @@ +1519,5 @@ > NS_ASSERTION(mDecoder, "SetCurrentTime failed: no decoder"); > return; > } > > + // Only refine the aTime while in the PrevSyncPoint or Accurate seeking mode. I think we can just pass currentTime to seek() in NextFrame mode, so most of the logic will apply without modification automatically. ::: dom/media/MediaDecoderStateMachine.cpp @@ +1116,5 @@ > > DECODER_LOG("MaybeStartPlayback() starting playback"); > mOnPlaybackEvent.Notify(MediaEventType::PlaybackStarted); > + > + //Note:: FrameStep -> Normal mode. It is wrong to switch back to NORMAL if current mode is AUDIO_CAPTURE. @@ +2948,5 @@ > // always in sync with the playing state of MediaSink. It will be started in > // MaybeStartPlayback() in the next cycle if necessary. > > + mPlaybackMode = aMode; > + mAudioCaptured = aMode == PlaybackMode::AUDIO_CAPTURE; We should remove mAudioCaptured since |mPlaybackMode==AUDIO_CAPTURE| is equivalent to mAudioCaptured. @@ +2993,5 @@ > > +void > +MediaDecoderStateMachine::SeekToNextFrame() > +{ > + this->mDropVideoUntilNextDiscontinuity = false; It is verbose to state |this|. @@ +2997,5 @@ > + this->mDropVideoUntilNextDiscontinuity = false; > + // so that the decoded video sample will be able to be pushed into VideoQueue > + // in the onVideoDeocded callback. > + > + if (mPlaybackMode != PlaybackMode::FRAME_STEP) { The if is already handled in SwitchPlaybackMode(). @@ +3001,5 @@ > + if (mPlaybackMode != PlaybackMode::FRAME_STEP) { > + SwitchPlaybackMode(PlaybackMode::FRAME_STEP); > + } > + > + if (!mMediaSink->HasUnplayedFrames(TrackInfo::kVideoTrack)) { It won't work for HasUnplayedFrames() is not implemented by most MediaSink sub-classes. @@ +3014,5 @@ > + > + if (mMediaSink->AdvanceFrame()) { > + UpdatePlaybackPosition(mMediaSink->GetPosition()); > + mOnPlaybackEvent.Notify(MediaEventType::Invalidate); > + SeekCompleted(); SeekCompleted() will call UpdatePlaybackPositionInternal() and overwrite your UpdatePlaybackPosition() above. @@ +3015,5 @@ > + if (mMediaSink->AdvanceFrame()) { > + UpdatePlaybackPosition(mMediaSink->GetPosition()); > + mOnPlaybackEvent.Notify(MediaEventType::Invalidate); > + SeekCompleted(); > + mDecodeToSeekTarget = true; What does this mean? @@ +3016,5 @@ > + UpdatePlaybackPosition(mMediaSink->GetPosition()); > + mOnPlaybackEvent.Notify(MediaEventType::Invalidate); > + SeekCompleted(); > + mDecodeToSeekTarget = true; > + DispatchDecodeTasksIfNeeded(); Why is this needed? @@ +3021,5 @@ > + return; > + } > + > + mCurrentSeek.RejectIfExists(__func__); > + return; This 'return' is not required. ::: dom/media/test/test_frame_step.html @@ +1,1 @@ > +<!DOCTYPE HTML> This is not a mochitest file. See dom/media/test/test_playback.html for an example.

(In reply to JW Wang [:jwwang] from comment #12) > @@ +3001,5 @@ > > + if (mPlaybackMode != PlaybackMode::FRAME_STEP) { > > + SwitchPlaybackMode(PlaybackMode::FRAME_STEP); > > + } > > + > > + if (!mMediaSink->HasUnplayedFrames(TrackInfo::kVideoTrack)) { > > It won't work for HasUnplayedFrames() is not implemented by most MediaSink > sub-classes. SwitchPlaybackMode(PlaybackMode::FRAME_STEP) guarantees it is using the FrameStepVideoSink. Maybe we can add a assert before this line? > @@ +3014,5 @@ > > + > > + if (mMediaSink->AdvanceFrame()) { > > + UpdatePlaybackPosition(mMediaSink->GetPosition()); > > + mOnPlaybackEvent.Notify(MediaEventType::Invalidate); > > + SeekCompleted(); > > SeekCompleted() will call UpdatePlaybackPositionInternal() and overwrite > your UpdatePlaybackPosition() above. Okay. However, I found that both operations do not fire the "timeupdate" event at the end because they are early returned at https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoder.cpp#1274. I need to find a way to dispatch the "timeupdate" event. > @@ +3015,5 @@ > > + if (mMediaSink->AdvanceFrame()) { > > + UpdatePlaybackPosition(mMediaSink->GetPosition()); > > + mOnPlaybackEvent.Notify(MediaEventType::Invalidate); > > + SeekCompleted(); > > + mDecodeToSeekTarget = true; > > What does this mean? > > @@ +3016,5 @@ > > + UpdatePlaybackPosition(mMediaSink->GetPosition()); > > + mOnPlaybackEvent.Notify(MediaEventType::Invalidate); > > + SeekCompleted(); > > + mDecodeToSeekTarget = true; > > + DispatchDecodeTasksIfNeeded(); > > Why is this needed? Sorry that I just follow the code at https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#1624. After double check, I think these two lines are both not needed.

(In reply to Tzuhao Kuo [:kaku] from comment #13) > SwitchPlaybackMode(PlaybackMode::FRAME_STEP) guarantees it is using the > FrameStepVideoSink. Maybe we can add a assert before this line? You may just call |mMediaSink->AdvanceFrame()| and decide to decode more frames if AdvanceFrame() fails.

Hi JW, Here is the first implementation patch, would like to have your review, thanks.

Comment on attachment 8708967 [details] [diff] [review] bug1235301 - HTMLMediaElement::SeekToNextFrame().patch Review of attachment 8708967 [details] [diff] [review]: ----------------------------------------------------------------- Please use mozReview where it is much easier to see diffs as patches evolve. ::: dom/html/HTMLMediaElement.cpp @@ +1518,5 @@ > return; > } > > + // Only refine the aTime while in the PrevSyncPoint or Accurate seeking mode. > + if (aSeekType != SeekTarget::NextFrame) { It would be clearer to extract the code in the if block into a function. ::: dom/media/MediaDecoder.cpp @@ +817,5 @@ > MediaDecoder::CallSeek(const SeekTarget& aTarget) > { > MOZ_ASSERT(NS_IsMainThread()); > + > + // In NextFrame seeking mode, continuous seekToNextFrame() operations should Do we really need this? I think MDSM has handled consecutive seeking gracefully. @@ +1270,5 @@ > } > > void > +MediaDecoder::UpdateLogicalPosition(MediaDecoderEventVisibility aEventVisibility, > + bool aForceUpdate /* = false */) I address this issue in a similar way in bug 1193124 comment 6. I think your change should also break test_fastSeek.html without bug 1239182 being fixed first. ::: dom/media/MediaDecoderStateMachine.cpp @@ +396,5 @@ > { > + if (aMode == PlaybackMode::FRAME_STEP) { > + RefPtr<media::MediaSink> rv = > + new FrameStepVideoSink(mAudioQueue, mVideoQueue, mVideoFrameContainer, > + mDuration.Ref().ref()); The duration might change as more video frames are decoded. It is not a good idea to pass the duration to FrameStepVideoSink to determine whether EOS is reached. @@ +611,5 @@ > mCurrentSeek.Exists(), mDropVideoUntilNextDiscontinuity, VideoQueue().IsFinished(), VideoQueue().GetSize()); > return > !HasVideo() || > (mCurrentSeek.Exists() && > + (mCurrentSeek.mTarget.mType != SeekTarget::NextFrame && !mDropVideoUntilNextDiscontinuity) && Why checking |mCurrentSeek.mTarget.mType != SeekTarget::NextFrame|? @@ +969,5 @@ > // in this case, we'll just decode forward. Bug 1026330. > mCurrentSeek.mTarget.mType = SeekTarget::Accurate; > } > if (mCurrentSeek.mTarget.mType == SeekTarget::PrevSyncPoint || > + mCurrentSeek.mTarget.mType == SeekTarget::NextFrame || Why checking this? @@ +2151,5 @@ > // for the seeking. > DECODER_LOG("A new seek came along while we were finishing the old one - staying in SEEKING"); > nextState = DECODER_STATE_SEEKING; > + } else if ((GetMediaTime() == Duration().ToMicroseconds() && !isLiveStream) || > + (mCurrentSeek.mTarget.mType == SeekTarget::NextFrame && Won't |GetMediaTime() == Duration().ToMicroseconds() && !isLiveStream| suffice? Since SeekTarget::NextFrame is basically a seek operation, we should avoid creating additional code paths for it as much as possible to reduce the complexity. @@ +2585,5 @@ > RefPtr<AudioData> audio(aSample->As<AudioData>()); > MOZ_ASSERT(audio && > mCurrentSeek.Exists() && > + (mCurrentSeek.mTarget.mType == SeekTarget::Accurate || > + mCurrentSeek.mTarget.mType == SeekTarget::NextFrame)); We don't need to call DropAudioUpToSeekTarget() when seek mode is NextFrame, right? @@ +2960,5 @@ > + mPlaybackMode = aMode; > + > + // The mAudioCaptured is a watchable. We keep this member and update it here > + // so that the MDSM::AdjustAudioThresholds() will be triggered. > + mAudioCaptured = aMode == PlaybackMode::AUDIO_CAPTURE; AdjustAudioThresholds() is removed in bug 948267. @@ +3008,5 @@ > +MediaDecoderStateMachine::SeekToNextFrame() > +{ > + // If the current media source has no video data, ignore the seek operation. > + if (!HasVideo()) { > + mCurrentSeek.RejectIfExists(__func__); MDSM won't leave DECODER_STATE_SEEKING if SeekCompleted() not called. Furthermore, we should reject it as soon as possible if HasVideo() is false. @@ +3031,5 @@ > + // 3) The operation failed because the next frame haven't been decoded. > + // Ask for decoding one more frame and hook this method to the VideoQueue's > + // push event. > + if (mMediaSink->AdvanceFrame() || VideoQueue().AtEndOfStream()) { > + mOnPlaybackEvent.Notify(MediaEventType::Invalidate); This is already handled by SeekCompleted(). @@ +3037,5 @@ > + return; > + } else { > + // connect VideoQueue().PushEvent() > + mVideoQueuePushListener = VideoQueue().PushEvent().Connect( > + mTaskQueue, this, &MediaDecoderStateMachine::SeekToNextFrame); MDSM might be shut down before SeekToNextFrame() is called next time. ::: dom/media/mediasink/FrameStepVideoSink.cpp @@ +55,5 @@ > + if (!mVideoQueue.AtEndOfStream()) { > + return mPosition; > + } > + > + return mDuration.ToMicroseconds(); It would be wrong if the start time of the last video frame is greater than the duration. ::: dom/media/mediasink/FrameStepVideoSink.h @@ +76,5 @@ > + MediaQueue<MediaData>& mAudioQueue; > + MediaQueue<MediaData>& mVideoQueue; > + VideoFrameContainer* mContainer; > + int64_t mPosition; > + const media::TimeUnit& mDuration; It would be better to store the duration by value instead of reference. ::: dom/media/mediasink/MediaSink.h @@ +118,5 @@ > // allocated by this sink should be released. > // Must be called after playback stopped. > virtual void Shutdown() {} > > + // Advance playback position and draw next frame. Please state the constraints of this function as we did for functions above. ::: dom/media/test/manifest.js @@ +278,5 @@ > + // Theora only oggz-chop stream > + { name:"bug482461-theora.ogv", type:"video/ogg", duration:4.138 }, > + // With first frame a "duplicate" (empty) frame. > + { name:"bug500311.ogv", type:"video/ogg", duration:1.96 }, > + space. ::: dom/webidl/HTMLMediaElement.webidl @@ +178,5 @@ > + * rate which means that the video element no longer sticks to real-time clock. > + * This method also lets authors use "frame" as unit to access the underlying > + * data, instead of by time. > + * > + * The SeekToNextFrame() is a kind of seek operation. space.

Based on the Bug 1261020, this issue should be implemented in a better way. So now we have SeekTask, we can first abstract the SeekTask class, and then implement two concrete classes, AccurateSeekTask and NextFrameSeekTask. The basic idea of NextFrameSeekTask is going to consume the MDSM::VideoQueue frame-by-frame in the rate controlled by users. So, while invoking a NextFrameSeekTask, we are not going to call MDSM::reset(), so don't we call the MDReader::ResetDecode(). However, this might lead to a situation that, when changing the MDSM to SEEKING state, the MDSM::mAudioDataRequest and MDSM::mVideoDataRequest might yet be resolved; and we DONT't want to discard them to keep the decoded data in the right order. JW suggests that we should make the MediaDecoderReaderWrapper as a proxy of requesting media data form MediaDecoderReader and be able to change callbacks at runtime. So that, while invoking a seek task, if there are still unsolved data requests, we can then re-direct those unsolved requests to the seek task.

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

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

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

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

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

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

Comment on attachment 8748534 [details] MozReview Request: Bug 1235301 part 0 - fix SeekTask; r=jwwang https://reviewboard.mozilla.org/r/50363/#review47181

Comment on attachment 8748535 [details] MozReview Request: Bug 1235301 part 1 - abstract the SeekTask class; r=jwwang https://reviewboard.mozilla.org/r/50365/#review47187 ::: dom/media/AccurateSeekTask.h:18 (Diff revision 1) > - bool mNeedToStopPrerollingVideo; > -}; > - > -class SeekTask { > > - NS_INLINE_DECL_THREADSAFE_REFCOUNTING(SeekTask) > + friend class SeekTask; Why do we need a friend class?

https://reviewboard.mozilla.org/r/50365/#review47187 > Why do we need a friend class? Because SeekTask::CreateSeekTask() calls the AccurateSeekTask's private constructor.

Comment on attachment 8748537 [details] MozReview Request: Bug 1235301 part 2 - implement NextFrameSeekTask; r=jwwang https://reviewboard.mozilla.org/r/50369/#review47193 ::: dom/media/MediaDecoderStateMachine.cpp:1492 (Diff revision 1) > > // Create a new SeekTask instance for the incoming seek task. > mSeekTask = SeekTask::CreateSeekTask(mDecoderID, OwnerThread(), > mReader.get(), Move(aSeekJob), > - mInfo, Duration(), GetMediaTime()); > + mInfo, Duration(), GetMediaTime(), > + mMediaSink.get()); It defeats the purpose of the factory method when different SeekTask subclasses take different parameters to construct. I would prefer let MDSM create the SeekTask subclass where it sees fit. ::: dom/media/NextFrameSeekTask.cpp:108 (Diff revision 1) > + // seek task object's callback is still be registered to the wrapper and once > + // the wrapper calls this seek object's callback, the callback will try to > + // resolve the mSeekTaskPromise which is yet ensure-d. Instead, if there is a > + // pending video request, we wait for it. > + // Note that we don't care the audio case. > + if ((mVideoSink->AdvanceFrame() || mVideoSink->VideoQueue().AtEndOfStream()) This breaks the encapsulation of what a SeekTask should do. It should be the job of MDSM to choose which frame to render after seeking.

Comment on attachment 8748536 [details] MozReview Request: Bug 1235301 part 2 - implement FrameStepVideoSink; r?jwwang https://reviewboard.mozilla.org/r/50367/#review47195 We should be able to do without FrameStepVideoSink for MDSM+NextFrameSeekTask should do the job.

Comment on attachment 8748538 [details] MozReview Request: Bug 1235301 part 3 - export HTMLMediaElement::seekToNextFrame(); r=jwwang https://reviewboard.mozilla.org/r/50371/#review47197 ::: dom/html/HTMLMediaElement.cpp:1569 (Diff revision 1) > // mDecoder must always be set in order to reach this point. > NS_ASSERTION(mDecoder, "SetCurrentTime failed: no decoder"); > return; > } > > + // Only refine the aTime while in the PrevSyncPoint or Accurate seeking mode. Why can't we clamp seek target when seek type is NextFrame?

Comment on attachment 8748535 [details] MozReview Request: Bug 1235301 part 1 - abstract the SeekTask class; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50365/diff/1-2/

Comment on attachment 8748537 [details] MozReview Request: Bug 1235301 part 2 - implement NextFrameSeekTask; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50369/diff/1-2/

Comment on attachment 8748538 [details] MozReview Request: Bug 1235301 part 3 - export HTMLMediaElement::seekToNextFrame(); r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50371/diff/1-2/

Comment on attachment 8748539 [details] MozReview Request: Bug 1235301 part 5 - mochitest;r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50373/diff/1-2/

Comment on attachment 8748535 [details] MozReview Request: Bug 1235301 part 1 - abstract the SeekTask class; r=jwwang https://reviewboard.mozilla.org/r/50365/#review47991 ::: dom/media/SeekTask.cpp:8 (Diff revision 2) > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "SeekTask.h" > +#include "AccurateSeekTask.h" Do we need to include "AccurateSeekTask.h" here?

Comment on attachment 8748537 [details] MozReview Request: Bug 1235301 part 2 - implement NextFrameSeekTask; r=jwwang https://reviewboard.mozilla.org/r/50369/#review47993 ::: dom/media/NextFrameSeekTask.h:17 (Diff revision 2) > + > +namespace mozilla { > +namespace media { > + > +class MediaSink; > +class FrameStepVideoSink; These forward declarations are not used. ::: dom/media/NextFrameSeekTask.h:86 (Diff revision 2) > + MediaQueue<MediaData>& mVideoQueue; > + > + /* > + * Internal state. > + */ > + int64_t const mCurrentTimeBeforeSeek; The convention is like |const int| or |const bool|. ::: dom/media/NextFrameSeekTask.cpp:98 (Diff revision 2) > +FindNextFrame(MediaQueue<MediaData>& aQueue, int64_t aTime) > +{ > + AutoTArray<RefPtr<MediaData>, 16> frames; > + aQueue.GetFirstElements(aQueue.GetSize(), &frames); > + for (auto&& frame : frames) { > + VideoData* v = frame->As<VideoData>(); You don't need cast. ::: dom/media/NextFrameSeekTask.cpp:122 (Diff revision 2) > +static void > +DropAllFrames(MediaQueue<MediaData>& aQueue) { > + while(aQueue.GetSize() > 0) { > + RefPtr<MediaData> releaseMe = aQueue.PopFront(); > + } > + return; You don't need this |return|. ::: dom/media/NextFrameSeekTask.cpp:154 (Diff revision 2) > +{ > + AssertOwnerThread(); > + > + if (!HasVideo()) { > + SeekTaskRejectValue val; > + return SeekTask::SeekTaskPromise::CreateAndReject(val, __func__); This will raise a decode error. MDSM should do nothing instead of erroring out. ::: dom/media/NextFrameSeekTask.cpp:168 (Diff revision 2) > + // seek task object's callback is still be registered to the wrapper and once > + // the wrapper calls this seek object's callback, the callback will try to > + // resolve the mSeekTaskPromise which is yet ensure-d. Instead, if there is a > + // pending video request, we wait for it. > + // Note that we don't care the audio case. > + if ((mVideoQueue.PeekFront().get() || mVideoQueue.AtEndOfStream()) PeekFront() can be replaced by |GetSize()>0|. When mVideoQueue.AtEndOfStream() is true, there is no way for video data request to be pending, right? ::: dom/media/NextFrameSeekTask.cpp:169 (Diff revision 2) > + // the wrapper calls this seek object's callback, the callback will try to > + // resolve the mSeekTaskPromise which is yet ensure-d. Instead, if there is a > + // pending video request, we wait for it. > + // Note that we don't care the audio case. > + if ((mVideoQueue.PeekFront().get() || mVideoQueue.AtEndOfStream()) > + && (strcmp(VideoRequestStatus(), "pending") != 0)) { Can you say |!mReader->IsRequestingVidoeData()| instead? What if |VideoRequestStatus()| is "waiting"? ::: dom/media/NextFrameSeekTask.cpp:171 (Diff revision 2) > + // pending video request, we wait for it. > + // Note that we don't care the audio case. > + if ((mVideoQueue.PeekFront().get() || mVideoQueue.AtEndOfStream()) > + && (strcmp(VideoRequestStatus(), "pending") != 0)) { > + UpdateSeekTargetTime(); > + SeekTaskResolveValue val = SeekTaskResolveValue(); // Make sure all the default values are "false" and "nullptr" Can't |SeekTaskResolveValue val;| do the job? ::: dom/media/NextFrameSeekTask.cpp:229 (Diff revision 2) > + > + SAMPLE_LOG("Queueing video task - queued=%i, decoder-queued=%o, skip=%i, time=%lld", > + !!mSeekedVideoData, mReader->SizeOfVideoQueueInFrames(), skipToNextKeyFrame, > + currentTime.ToMicroseconds()); > + > + mReader->RequestVideoData(skipToNextKeyFrame, currentTime); Just say |RequestVideoData(false, media::TimeUnit())|. ::: dom/media/NextFrameSeekTask.cpp:345 (Diff revision 2) > + CheckIfSeekComplete(); > + } > +} > + > +void > +NextFrameSeekTask::SetMediaDecoderReaderWrapperCallback() AssertOwnerThread(). ::: dom/media/NextFrameSeekTask.cpp:353 (Diff revision 2) > + mReader->SetVideoCallback(this, &NextFrameSeekTask::OnVideoDecoded, > + &NextFrameSeekTask::OnVideoNotDecoded); > + > + // Register dummy callbcak for audio decoding since we don't need to handle > + // the decoded audio samples. > + mAudioCallbackID = mReader->SetAudioCallback( We will lose audio data by doing so. ::: dom/media/NextFrameSeekTask.cpp:362 (Diff revision 2) > + DECODER_LOG("NextFrameSeekTask set audio callbacks: mVideoCallbackID = %d\n", (int)mAudioCallbackID); > + DECODER_LOG("NextFrameSeekTask set video callbacks: mVideoCallbackID = %d\n", (int)mVideoCallbackID); > +} > + > +void > +NextFrameSeekTask::CancelMediaDecoderReaderWrapperCallback() AssertOwnerThread(). ::: dom/media/NextFrameSeekTask.cpp:372 (Diff revision 2) > + DECODER_LOG("NextFrameSeekTask cancel video callbacks: mVideoCallbackID = %d\n", (int)mVideoCallbackID); > + mReader->CancelVideoCallback(mVideoCallbackID); > +} > + > +void > +NextFrameSeekTask::UpdateSeekTargetTime() AssertOwnerThread(). ::: dom/media/NextFrameSeekTask.cpp:379 (Diff revision 2) > + RefPtr<MediaData> data = mVideoQueue.PeekFront(); > + if (data) { > + mSeekJob.mTarget.SetTime(TimeUnit::FromMicroseconds(data->mTime)); > + } else if (mSeekedVideoData) { > + mSeekJob.mTarget.SetTime(TimeUnit::FromMicroseconds(mSeekedVideoData->mTime)); > + } else if (mVideoQueue.AtEndOfStream()) { We should jump to the end so 'ended' event can be fired, right?

Comment on attachment 8748538 [details] MozReview Request: Bug 1235301 part 3 - export HTMLMediaElement::seekToNextFrame(); r=jwwang https://reviewboard.mozilla.org/r/50371/#review48021 ::: dom/media/MediaDecoder.cpp:824 (Diff revision 2) > > int64_t timeUsecs = TimeUnit::FromSeconds(aTime).ToMicroseconds(); > > + // While in NextFrame seeking mode, the new mLogicalPosition will be updated > + // after the seeking operation is done. > mLogicalPosition = aTime; I can't see why mLogicalPosition is updated after seeking only when seek type is 'NextFrame'. ::: dom/media/MediaDecoderStateMachine.cpp:1499 (Diff revision 2) > + // seek target will later be used to update the mCurrentPosition. > + // So that, in the MDSM::SeekCompleted() the next state will be > + // DECODER_STATE_COMPLETED. > + if (aSeekJob.mTarget.IsNextFrame()) { > + if (mMinimizePreroll) { > + mMinimizePreroll = false; Why is this? ::: dom/media/MediaDecoderStateMachine.cpp:1506 (Diff revision 2) > + > + // This is the only situation which we know the seek target in advance > + // before starting a NextFrameSeekTask. Otherwise, the seek target will be > + // updated once it is known in the NextFrameSeekTask. > + if (VideoQueue().AtEndOfStream()) { > + aSeekJob.mTarget.SetTime(Duration()); Can't we do that inside NextFrameSeekTask which will change seek target anyway? ::: dom/media/MediaDecoderStateMachine.cpp:1519 (Diff revision 2) > - mInfo, Duration(), GetMediaTime()); > + mInfo, Duration(), GetMediaTime()); > + } else if (aSeekJob.mTarget.IsNextFrame()) { > + mSeekTask = new NextFrameSeekTask(mDecoderID, OwnerThread(), mReader.get(), > + Move(aSeekJob), mInfo, Duration(), > + GetMediaTime(), AudioQueue(), VideoQueue()); > + } Assert we exhaust all seek types at the end. ::: dom/webidl/HTMLMediaElement.webidl:182 (Diff revision 2) > readonly attribute double computedVolume; > [Pref="media.useAudioChannelService.testing"] > readonly attribute boolean computedMuted; > }; > + > +/* Need a DOM peer to review this change. It will be better to split this change to a new patch.

Comment on attachment 8748539 [details] MozReview Request: Bug 1235301 part 5 - mochitest;r=jwwang https://reviewboard.mozilla.org/r/50373/#review48027 ::: dom/media/test/test_seekToNextFrame.html:40 (Diff revision 2) > + > + v.src = test.name; > + v.name = test.name; > + > + var check = function(test, v) { return function() { > +console.log(test.name + " loadedmetadata()"); indentation. ::: dom/media/test/test_seekToNextFrame.html:48 (Diff revision 2) > +ok(true, " loadedmetadata(): before v.SeekToNextFrame();"); > + v.SeekToNextFrame(); > +ok(true, " loadedmetadata(): after v.SeekToNextFrame();"); > + }}(test, v); > + > + var noLoad = function(test, v) { return function() { We don't want to test 'load' event in this test, right? ::: dom/media/test/test_seekToNextFrame.html:93 (Diff revision 2) > + > + v.seenSuspend = true; > + mayFinish(); > + }}(test, v); > + > + var timeUpdate = function(test, v) { return function() { Please remove all tests that are not related to NextFrameSeek.

https://reviewboard.mozilla.org/r/50369/#review47993 > PeekFront() can be replaced by |GetSize()>0|. > > When mVideoQueue.AtEndOfStream() is true, there is no way for video data request to be pending, right? Yes. I could separate these two cases. > Can you say |!mReader->IsRequestingVidoeData()| instead? > > What if |VideoRequestStatus()| is "waiting"? Hmm..., SeekTask has no way to know the "waitting" situation because MDSM and SeekTask use different mAudioWaitRequest/mVideoWaitRequest. We could solve this issue by integrating the "WaitForMediaData" into MediaDecoderReaderWraper too (just like RequestAudioData()/RequestVideoData()) or we could remove the whole "wait for data" operation (Bug 1270699). > Can't |SeekTaskResolveValue val;| do the job? No, |SeekTaskResolveValue val;| cannot. The reason is that the data members of SeekTaskResolveValue do not memtioned in the default consturctor's intializer list so that thery are initialized to indeterminate values. We can just write a default consturctor with all members are initialized in the initializer list, then |SeekTaskResolveValue val;| should works. > We will lose audio data by doing so. Okay..., then I will implement a full audio callback. > We should jump to the end so 'ended' event can be fired, right? We don't need to do that because, in this case, the mSeekJob.mTarget's time has already been set to the media source's duration in the MDSM::InitializeSeek(). Also, setting the mSeekJob.mTarget's time to the media source's duration won't make the MDSM to fire a 'ended' event because the MDSM::SeekCompleted() checks |GetMediaTime() == Duration()...| (https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#1994) to fire the 'ended' event instead of checking the mSeekJob.mTarget's time to the duration. And for the |GetMediaTime() == Duration()...| to success, we have to update the MDSM::mCurrentPosition to the media source's duration at the MDSM::InitializeSeek().

https://reviewboard.mozilla.org/r/50371/#review48021 > I can't see why mLogicalPosition is updated after seeking only when seek type is 'NextFrame'. Because we don't know the true target time, the aTime here was set by the HTMLMediaElement::CurrentTime() which calls MediaDecoder::GetCurrentTime() which returns mLogicalPosition. So this statement actually does not update any information. > Why is this? To my understanding, make this false indicates that MDSM could buffer more data, right? > Can't we do that inside NextFrameSeekTask which will change seek target anyway? My purpose here is to make sure the following method call |UpdatePlaybackPositionInternal(mSeekTask->GetSeekJob().mTarget.GetTime().ToMicroseconds());| updates the MDSM::mCurrentPosition to the media source's duration so that the 'ended' event will be fired at the MDSM::SeelCompleted(). > Need a DOM peer to review this change. It will be better to split this change to a new patch. Sure, may I have your suggestion on the DOM peer who is familiar with the media part?

https://reviewboard.mozilla.org/r/50369/#review47993 > Hmm..., SeekTask has no way to know the "waitting" situation because MDSM and SeekTask use different mAudioWaitRequest/mVideoWaitRequest. > We could solve this issue by integrating the "WaitForMediaData" into MediaDecoderReaderWraper too (just like RequestAudioData()/RequestVideoData()) or we could remove the whole "wait for data" operation (Bug 1270699). I will prefer to wait for bug 1270699 which makes code much simpler. > No, |SeekTaskResolveValue val;| cannot. The reason is that the data members of SeekTaskResolveValue do not memtioned in the default consturctor's intializer list so that thery are initialized to indeterminate values. > > We can just write a default consturctor with all members are initialized in the initializer list, then |SeekTaskResolveValue val;| should works. Doesn't |SeekTaskResolveValue val = SeekTaskResolveValue()| invoke the default constructor which does nothing? > We don't need to do that because, in this case, the mSeekJob.mTarget's time has already been set to the media source's duration in the MDSM::InitializeSeek(). > > Also, setting the mSeekJob.mTarget's time to the media source's duration won't make the MDSM to fire a 'ended' event because the MDSM::SeekCompleted() checks |GetMediaTime() == Duration()...| (https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#1994) to fire the 'ended' event instead of checking the mSeekJob.mTarget's time to the duration. And for the |GetMediaTime() == Duration()...| to success, we have to update the MDSM::mCurrentPosition to the media source's duration at the MDSM::InitializeSeek(). It looks like we should check |newCurrentTime == Duration()...| instead of |GetMediaTime() == Duration()...|. Please file a new bug for that.

https://reviewboard.mozilla.org/r/50371/#review48021 > Because we don't know the true target time, the aTime here was set by the HTMLMediaElement::CurrentTime() which calls MediaDecoder::GetCurrentTime() which returns mLogicalPosition. So this statement actually does not update any information. Please just remove the comment to avoid confusion. > To my understanding, make this false indicates that MDSM could buffer more data, right? The flag is used to control whether MDSM should decode more audio/video data. It should has nothing to do with the concerned SeekTask here, right? > My purpose here is to make sure the following method call |UpdatePlaybackPositionInternal(mSeekTask->GetSeekJob().mTarget.GetTime().ToMicroseconds());| updates the MDSM::mCurrentPosition to the media source's duration so that the 'ended' event will be fired at the MDSM::SeelCompleted(). See my previous comment about |newCurrentTime == Duration()...| which should fix the problem here.

https://reviewboard.mozilla.org/r/50369/#review47993 > Doesn't |SeekTaskResolveValue val = SeekTaskResolveValue()| invoke the default constructor which does nothing? As far as I know, bases on that we don't write the default CTOR for the SeekTaskResolveValue: (1) |SeekTaskResolveValue val = SeekTaskResolveValue()| invokes "value initialization" which then has the SeekTaskResolveValue's scalar member be "zero-initialized". "Zero initialization" initializes scalar types to the value which is converted from 0. (2) |SeekTaskResolveValue val;| invokes "default initialization" which has the SeekTaskResolveValue's scalar member be "default-initialized" too. "Default initialization" initializes NON-STATIC scalar type to indeterminate values. Whatever, I will then write the default constructor with intializer list by my self and then use |SeekTaskResolveValue val;|. I think this is much easier to understand...

https://reviewboard.mozilla.org/r/50369/#review47993 > As far as I know, bases on that we don't write the default CTOR for the SeekTaskResolveValue: > > (1) |SeekTaskResolveValue val = SeekTaskResolveValue()| invokes "value initialization" which then has the SeekTaskResolveValue's scalar member be "zero-initialized". "Zero initialization" initializes scalar types to the value which is converted from 0. > > (2) |SeekTaskResolveValue val;| invokes "default initialization" which has the SeekTaskResolveValue's scalar member be "default-initialized" too. "Default initialization" initializes NON-STATIC scalar type to indeterminate values. > > Whatever, I will then write the default constructor with intializer list by my self and then use |SeekTaskResolveValue val;|. I think this is much easier to understand... I guess you can do |SeekTaskResolveValue val = {}| to zero-initialize all members which is less confusing.

https://reviewboard.mozilla.org/r/50369/#review47993 > I guess you can do |SeekTaskResolveValue val = {}| to zero-initialize all members which is less confusing. Yes, looks like it is a special case of copy-list-initialization with empty braced-init-list which will then reduce to value-initizlization (and so zero-initialize all members). Reference: http://en.cppreference.com/w/cpp/language/list_initialization. I personally think these are all confusing...

https://reviewboard.mozilla.org/r/50369/#review47993 > It looks like we should check |newCurrentTime == Duration()...| instead of |GetMediaTime() == Duration()...|. Please file a new bug for that. Here you are: bug 1271581.

https://reviewboard.mozilla.org/r/50369/#review47993 > I will prefer to wait for bug 1270699 which makes code much simpler. Per discussion offline. The bug 1270699 still takes time, we instead take the other approach, Bug 1274192.

https://reviewboard.mozilla.org/r/50371/#review48021 > The flag is used to control whether MDSM should decode more audio/video data. It should has nothing to do with the concerned SeekTask here, right? So that is my goal, I want the MDSM to decode more video data. More video data are queued, the SeekToNextFrame() operation responses quicker.

Comment on attachment 8748534 [details] MozReview Request: Bug 1235301 part 0 - fix SeekTask; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50363/diff/1-2/

Comment on attachment 8748535 [details] MozReview Request: Bug 1235301 part 1 - abstract the SeekTask class; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50365/diff/2-3/

Comment on attachment 8748537 [details] MozReview Request: Bug 1235301 part 2 - implement NextFrameSeekTask; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50369/diff/2-3/

Comment on attachment 8748538 [details] MozReview Request: Bug 1235301 part 3 - export HTMLMediaElement::seekToNextFrame(); r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50371/diff/2-3/

Comment on attachment 8748539 [details] MozReview Request: Bug 1235301 part 5 - mochitest;r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50373/diff/2-3/

HTMLMediaElement.webidl file for details.

Comment on attachment 8755694 [details] [diff] [review] Bug 1235301 part 4 - webidl; r?jwwang, bz Please refere to the comment in the HTMLMediaElement.webidl file for details.

Comment on attachment 8748537 [details] MozReview Request: Bug 1235301 part 2 - implement NextFrameSeekTask; r=jwwang https://reviewboard.mozilla.org/r/50369/#review51412 ::: dom/media/NextFrameSeekTask.cpp:208 (Diff revisions 2 - 3) > SAMPLE_LOG("EnsureVideoDecodeTaskQueued isDecoding=%d status=%s", > IsVideoDecoding(), VideoRequestStatus()); > > if (!IsVideoDecoding() || > - mReader->IsRequestingVidoeData() || > - mVideoWaitRequest.Exists()) { > + mReader->IsRequestingVideoData() || > + mReader->IsRequestingAudioData()) { Why do we return when an audio request is pending? ::: dom/media/NextFrameSeekTask.cpp:224 (Diff revisions 2 - 3) > AssertOwnerThread(); > - if (mReader->IsRequestingVidoeData()) { > - MOZ_DIAGNOSTIC_ASSERT(!mVideoWaitRequest.Exists()); > + > + if (mReader->IsRequestingVideoData()) { > + MOZ_DIAGNOSTIC_ASSERT(!mReader->IsRequestingAudioData()); > return "pending"; > - } else if (mVideoWaitRequest.Exists()) { > + } else if (mReader->IsRequestingAudioData()) { What is audio doing here in a query about video? ::: dom/media/NextFrameSeekTask.cpp:234 (Diff revisions 2 - 3) > AssertOwnerThread(); > - //These two variables are not used in the SEEKING state. > - const bool skipToNextKeyFrame = false; > - const media::TimeUnit currentTime = media::TimeUnit::FromMicroseconds(0); > - > SAMPLE_LOG("Queueing video task - queued=%i, decoder-queued=%o, skip=%i, time=%lld", We don't have to print "skip=%i, time=%lld" at all. ::: dom/media/NextFrameSeekTask.cpp:247 (Diff revisions 2 - 3) > -NextFrameSeekTask::IsVideoSeekComplete() > +NextFrameSeekTask::IsAudioSeekComplete() > { > AssertOwnerThread(); > + SAMPLE_LOG("IsAudioSeekComplete() curTarVal=%d aqFin=%d aqSz=%d req=%d wait=%d", > + mSeekJob.Exists(), mIsAudioQueueFinished, !!mSeekedAudioData, > + !!mReader->IsRequestingAudioData(), !!mReader->IsWaitingAudioData()); You don't have to prepend "!!" because mReader->IsRequestingAudioData() already returns a bool. ::: dom/media/NextFrameSeekTask.cpp:373 (Diff revision 3) > + // We've received a sample from a previous decode. Discard it. > + return; > + } > + > + if (aReason == MediaDecoderReader::DECODE_ERROR) { > + if (mVideoQueue.GetSize() > 0) { This case will not happen because we only request video when we don't have the target frame, right?

Comment on attachment 8748538 [details] MozReview Request: Bug 1235301 part 3 - export HTMLMediaElement::seekToNextFrame(); r=jwwang https://reviewboard.mozilla.org/r/50371/#review51420

Comment on attachment 8748539 [details] MozReview Request: Bug 1235301 part 5 - mochitest;r=jwwang https://reviewboard.mozilla.org/r/50373/#review51422 ::: dom/media/test/manifest.js:279 (Diff revision 3) > > // Invalid file > { name:"bogus.duh", type:"bogus/duh", duration:Number.NaN }, > ]; > > +var gSeekToNextFrameTests = [ Can you reuse gPlayTests and filter out those without video?

https://reviewboard.mozilla.org/r/50369/#review51412 > Why do we return when an audio request is pending? Sorry, it should be mReader->IsWaitingVideoData(). > What is audio doing here in a query about video? They should be the video counterparts...... > This case will not happen because we only request video when we don't have the target frame, right? The request might be filed by MDSM not the NextFrameSeekTask itself. So, the NextFrameSeekTask might has already found its target in the VideoQueue but still waits the video decoding request (which is filed by the MDSM) to be resolved.

https://reviewboard.mozilla.org/r/50371/#review51450 Will update this patch later. ::: dom/media/MediaDecoderStateMachine.cpp:1580 (Diff revision 3) > + if (aSeekJob.mTarget.IsNextFrame() && !HasVideo()) { > + // Just ignore this seek task. > + return; > + } We cannot reutrn without initializaing mSeekTask because the mSeekTask will immediactely be used after returning form this method in the MDSM::Seek(). Instead checking this criteria here, we should move this check to the HTMLMediaElement::Seek() where we also do checking "seekable".

Please refere to the comment in the HTMLMediaElement.webidl file for details. Review commit: https://reviewboard.mozilla.org/r/54810/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54810/

Comment on attachment 8748537 [details] MozReview Request: Bug 1235301 part 2 - implement NextFrameSeekTask; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50369/diff/3-4/

Comment on attachment 8748538 [details] MozReview Request: Bug 1235301 part 3 - export HTMLMediaElement::seekToNextFrame(); r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50371/diff/3-4/

Comment on attachment 8748539 [details] MozReview Request: Bug 1235301 part 5 - mochitest;r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50373/diff/3-4/

(In reply to Tzuhao Kuo [:kaku] from comment #62) > Comment on attachment 8748538 [details] > MozReview Request: Bug 1235301 part 3 - export > HTMLMediaElement::SeekToNextFrame(); r?jwwang > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/50371/diff/3-4/ @jw, this one needs your review again!

Comment on attachment 8748534 [details] MozReview Request: Bug 1235301 part 0 - fix SeekTask; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50363/diff/2-3/

Comment on attachment 8748535 [details] MozReview Request: Bug 1235301 part 1 - abstract the SeekTask class; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50365/diff/3-4/

Comment on attachment 8748537 [details] MozReview Request: Bug 1235301 part 2 - implement NextFrameSeekTask; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50369/diff/4-5/

Comment on attachment 8748538 [details] MozReview Request: Bug 1235301 part 3 - export HTMLMediaElement::seekToNextFrame(); r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50371/diff/4-5/

Comment on attachment 8755784 [details] MozReview Request: Bug 1235301 part 4 - webidl; r=ehsan Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54810/diff/1-2/

Comment on attachment 8748539 [details] MozReview Request: Bug 1235301 part 5 - mochitest;r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50373/diff/4-5/

(In reply to Tzuhao Kuo [:kaku] from comment #64) > (In reply to Tzuhao Kuo [:kaku] from comment #62) > > Comment on attachment 8748538 [details] > > MozReview Request: Bug 1235301 part 3 - export > > HTMLMediaElement::SeekToNextFrame(); r?jwwang > > > > Review request updated; see interdiff: > > https://reviewboard.mozilla.org/r/50371/diff/3-4/ > > @jw, this one needs your review again! Looks good to me.

Comment on attachment 8748537 [details] MozReview Request: Bug 1235301 part 2 - implement NextFrameSeekTask; r=jwwang https://reviewboard.mozilla.org/r/50369/#review51668

I tried a bit to find some spec discussion around SeekToNextFrame(). The only thing that I found was https://github.com/w3c/mediacapture-worker/issues/33, where padenot and Domenic are both suggesting against this way of doing things. Can you please let me know what happened to that discussion and why you chose to proceed with SeekToNextFrame()? I would like to make sure that there is a path towards standardizing this API if we're going to implement it. Some other issues: * Please send an intent to implement email per https://wiki.mozilla.org/WebAPI/ExposureGuidelines. Make sure to mention that you're enabling this API on non-release channels. * It is more common for APIs to be camelCased not CamelCased, so you'd need to rename this to seekToNextFrame(). * The API that you're adding is synchronous which doesn't sound like what we want at all, it needs to return a Promsie<void> or some such to signal when the seek operation is finished. * FWIW, I agree with Paul and Domenic that a streams based approach would be nicer. :-)

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fa6727ad16c Compile failed on Window platform. The root cause is that the MediaStream::GetCurrentTime() conflicts with the macro GetCurrentTime in WinBas'h. I will add a patch to undefine the symbol in front of the MediaStream class definition.

I can see the benefit of having this functionality as part of a larger streams API, but we don't want to wait for the standards group to settle on a design before landing this code. We'd like to land this API so that we can use it for writing reftests. It will be useful to elicit feedback from authors. So we'd like to land this API as an experimental feature, that is behind a pref that is only enabled in Nightly and Developer Edition builds. Also, the infrastructure work here will likely be useful if the API changes to a stream-based approach. Indeed a streams based approach can likely be polyfilled using this API. I agree that this API should be camelCased, and it definitely should be promise based.

Thanks Chris. @Ehsan, so we proceeded this because the needs from media team, especially that this API could help testing. Also, we want to have an experimental interface for collecting feedback from public. The WHATWG Stream spec is still evolving, we should keep an eye on it without blocking our experiments.

(In reply to Chris Pearce (:cpearce) from comment #75) > I agree that this API should be camelCased, and it definitely should be > promise based. We can change the API to be promise-based in next bug so we can land this one as soon as possible to alleviate the pain of rebasing. Of cos, the pref will not be enabled (in Nightly and Dev Edition) until promise change is completed in next bug.

Comment on attachment 8755784 [details] MozReview Request: Bug 1235301 part 4 - webidl; r=ehsan https://reviewboard.mozilla.org/r/54810/#review52454 Since this is not intended to be shipped, sounds good to land, but please address the comments below. Thanks! ::: dom/webidl/HTMLMediaElement.webidl:187 (Diff revision 2) > + > +/* > + * The SeekToNextFrame() method provides a way to access a video element's video > + * frames one by one without going through the realtime playback. So, it lets > + * authors use "frame" as unit to access the video element's underlying data, > + * instead of "time". Please note that this is an experimental Mozilla extension here. ::: dom/webidl/HTMLMediaElement.webidl:213 (Diff revision 2) > + * currentTime to the duration of the media source and dispatches a "seeked" > + * event and an "ended" event. > + */ > +partial interface HTMLMediaElement { > + [Throws, Pref="media.seekToNextFrame"] > + void SeekToNextFrame(); Please file a follow-up bug for making this return a Promise<void>, and include a comment here pointing to that bug. Also, please rename the method to "seekToNextFrame". ::: modules/libpref/init/all.js:5354 (Diff revision 2) > #else > pref("dom.node.rootNode.enabled", true); > #endif > + > +#ifdef RELEASE_BUILD > +pref("media.seekToNextFrame", false); I agree with Xidorn's comment on dev-platform about "media.seekToNextFrame.enabled" being a better name here.

Comment on attachment 8748534 [details] MozReview Request: Bug 1235301 part 0 - fix SeekTask; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50363/diff/3-4/

Comment on attachment 8748535 [details] MozReview Request: Bug 1235301 part 1 - abstract the SeekTask class; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50365/diff/4-5/

Comment on attachment 8748537 [details] MozReview Request: Bug 1235301 part 2 - implement NextFrameSeekTask; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50369/diff/5-6/

Comment on attachment 8748538 [details] MozReview Request: Bug 1235301 part 3 - export HTMLMediaElement::seekToNextFrame(); r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50371/diff/5-6/

Comment on attachment 8755784 [details] MozReview Request: Bug 1235301 part 4 - webidl; r=ehsan Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54810/diff/2-3/

Comment on attachment 8748539 [details] MozReview Request: Bug 1235301 part 5 - mochitest;r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50373/diff/5-6/

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=981930c0047d Thanks for all the reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/26d53d0eaf66 https://hg.mozilla.org/integration/mozilla-inbound/rev/a462681c5a77 https://hg.mozilla.org/integration/mozilla-inbound/rev/7e82fc4bc49e https://hg.mozilla.org/integration/mozilla-inbound/rev/c70fa3dc42c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0f90c4b83a https://hg.mozilla.org/integration/mozilla-inbound/rev/8895926c8142

https://hg.mozilla.org/mozilla-central/rev/26d53d0eaf66 https://hg.mozilla.org/mozilla-central/rev/a462681c5a77 https://hg.mozilla.org/mozilla-central/rev/7e82fc4bc49e https://hg.mozilla.org/mozilla-central/rev/c70fa3dc42c0 https://hg.mozilla.org/mozilla-central/rev/ad0f90c4b83a https://hg.mozilla.org/mozilla-central/rev/8895926c8142

Added this page: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/seekToNextFrame Updated https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement to include this method (labeled experimental and non-standard). Updated Firefox 49 for developers. I would appreciate a quick review of the main article's content (https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/seekToNextFrame) to ensure that it's accurate and that my warnings not to use this in production code are strong enough. I will consider this documentation completed unless I hear otherwise.

(In reply to Eric Shepherd [:sheppy] from comment #88) > I would appreciate a quick review of the main article's content > (https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/ > seekToNextFrame) to ensure that it's accurate and that my warnings not to > use this in production code are strong enough. The content is correct to the scope of this bug. Bug 1276272 makes this feature a promise-based method and since there is a dev-doc-needed flag in the Bug 1276272, I think we will modify the content accordingly, right? I think the warnings are explicit and strong.

(In reply to Tzuhao Kuo [:kaku] from comment #89) > (In reply to Eric Shepherd [:sheppy] from comment #88) > The content is correct to the scope of this bug. Bug 1276272 makes this > feature a promise-based method and since there is a dev-doc-needed flag in > the Bug 1276272, I think we will modify the content accordingly, right? Correct. > I think the warnings are explicit and strong. Great. Thank you for your quick look at this!