1257107 - Don't create VideoData for frames while decoding to seek target

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

When we seek to a target time, we seek decoders to the time keyframe preceding the seek target, and decode forward up until the seek target. We never display the frames between the keyframe preceding the seek target, and the seek target. However, in the case of software decoding, we need to pay the price of copying the decoded video frame out of the decoder's memory and into a (possibly GPU memory) gfx Image, and in the case of hardware decoding, at least on Windows, we need to copy the image out of the decoder's memory in order to do an YUV to RGB colour conversion.

We could save time by eliminating this copy. While seeking, we can have the MediaFormatReader not output VideoData for frames before the seek target.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Someone on Taipei team is welcome to take this on.

Comment 2

[Alfredo Yang (:alfredo)]

Attachment: skip_frame_before_seek_target

My idea is to retrieve the target seek time and dropping frame if necessary before PDM Output().
I'll finish the whole patch if this is ok.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8737113 [details] [diff] [review]
skip_frame_before_seek_target

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

jya should offer feedback on this, he'll need to review it anyway.

I'm not super keen on the MediaDataDecoder calling out to the MediaFormatReader on every output... Feels like concerns are not properly separated... Maybe instead add a function to MediaDataDecoder interface SuppressOutputUntil(timestamp) instead?

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8737113 [details] [diff] [review]
skip_frame_before_seek_target

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

I would much prefer that we have a new member to MediaDataDecoder like SetThreshold() (or as cpearce suggested SuppressOutputUntil) and MediaDataDecoder have a new protected member that is Maybe<media::TimeUnit> mTargetThreshold

If mTargetThreshold is set, it becomes the responsibility of the MediaDataDecoder to drop all frames prior the value of mTargetThreshold. Once data is decoded past this value, then mTargetThreshold must be reset.

I would rename MediaDataDecoder::Flush as MediaDataDecoder::Reset to more likely represent what it's actually doing and remove ambiguities on what flush actually does.
MediaDataDecoder::Reset() would also reset mTargetThreshold.

I find this much nicer than having to query for every output the callback.

SetThreshold/SuppressOutputUntil would dispatch a task on the taskqueue setting mTargetThreshold to ensure mTargetThreshold is only ever accessed on the MediaDataDecoder's taskqueue.

Comment 5

[Alfredo Yang (:alfredo)]

Attachment: skip_decoded_frame_before_seek_target

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8745892 [details] [diff] [review]
skip_decoded_frame_before_seek_target

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

LGTM, but a few important points:

1- You must reset the threshold in reset as otherwise depending on the timing you could end up dropping frames when you don't want to.
2- There are important issues, in particular passing references across threads. This is not safe and can easily result in UAF

::: dom/media/MediaFormatReader.cpp
@@ +1537,5 @@
>    LOGV("Video seeked to %lld", aTime.ToMicroseconds());
>    mVideo.mSeekRequest.Complete();
>  
> +  auto& decoder = GetDecoderData(TrackInfo::kVideoTrack);
> +  decoder.mDecoder->SetSeekThreshold(mPendingSeekTime.ref());

you must make sure that there's a video track available otherwise this will cause a null deref

::: dom/media/platforms/PlatformDecoderModule.h
@@ +236,5 @@
>    virtual const char* GetDescriptionName() const = 0;
> +
> +  // Set a hint of seek target time to decoder. Decoder will drop any decoded
> +  // data which pts is smaller than this value.
> +  // Decoder may not honour this value. However, it'd be better that

may not honor.

Also, please add a note that the seek threshold must be reset upon a call to Reset() or upon reaching the seek target

@@ +238,5 @@
> +  // Set a hint of seek target time to decoder. Decoder will drop any decoded
> +  // data which pts is smaller than this value.
> +  // Decoder may not honour this value. However, it'd be better that
> +  // video decoder implements this API to improve seek performance.
> +  virtual nsresult SetSeekThreshold(media::TimeUnit& aTime) = 0;

const media::TimeUnit&

Is there value in returning nsresult ? would void be sufficient? seeing that even if the decoder does nothing it will still continue to work.

::: dom/media/platforms/agnostic/gmp/MediaDataDecoderProxy.cpp
@@ +106,5 @@
> +  MOZ_ASSERT(!IsOnProxyThread());
> +  MOZ_ASSERT(!mIsShutdown);
> +
> +  nsCOMPtr<nsIRunnable> task;
> +  task = NS_NewRunnableMethodWithArg<media::TimeUnit&>(mProxyDecoder, &MediaDataDecoder::SetSeekThreshold, aTime);

80 columns formatting

you certainly don't want to pass a reference there, as the original TimeUnit could have been destructed prior the task running.

you must pass the object by value and not by reference across threads.

::: dom/media/platforms/apple/AppleVDADecoder.cpp
@@ +298,5 @@
> +nsresult
> +AppleVDADecoder::SetSeekThreshold(media::TimeUnit& aTime)
> +{
> +  LOG("SetSeekThreshold %lld", aTime.ToMicroseconds());
> +  MonitorAutoLock mon(mMonitor);

can't you also dispatch a new task so you don't have to use the monitor in the output code?

::: dom/media/platforms/wmf/WMFMediaDataDecoder.cpp
@@ +262,5 @@
> +  MOZ_DIAGNOSTIC_ASSERT(!mIsShutDown);
> +
> +  RefPtr<WMFMediaDataDecoder> self = this;
> +  nsCOMPtr<nsIRunnable> runnable =
> +    NS_NewRunnableFunction([self, &aTime] () {

you do not want to capture a reference ; you must capture by value.

::: dom/media/platforms/wmf/WMFMediaDataDecoder.h
@@ +55,5 @@
>    virtual void ConfigurationChanged(const TrackInfo& aConfig) {}
>  
>    virtual const char* GetDescriptionName() const = 0;
>  
> +  nsresult SetSeekThreshold(media::TimeUnit& aTime) {

const media::TimeUnit&

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp
@@ +636,5 @@
>        }
> +      if (mSeekTargetThreshold.isSome()) {
> +        media::TimeUnit pts = GetSampleTime(sample);
> +        if (!pts.IsValid()) {
> +          continue;

shouldn't this be treated as error?

Comment 7

[Alfredo Yang (:alfredo)]

Attachment: skip_decoded_frame_before_seek_target

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8746985 [details] [diff] [review]
skip_decoded_frame_before_seek_target

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

r=me with comments addressed.

::: dom/media/platforms/PlatformDecoderModule.h
@@ +239,5 @@
> +  // data which pts is smaller than this value. This threshold needs to be clear
> +  // after reset decoder.
> +  // Decoder may not honor this value. However, it'd be better that
> +  // video decoder implements this API to improve seek performance.
> +  virtual void SetSeekThreshold(const media::TimeUnit& aTime) = 0;

It would be better to have a default version that does nothing. that way you don't have to do it for all the audio decoders etc... seeing that you're only implementing it for some video decoders

::: dom/media/platforms/agnostic/VPXDecoder.cpp
@@ +124,5 @@
>      NS_ASSERTION(img->fmt == VPX_IMG_FMT_I420 ||
>                   img->fmt == VPX_IMG_FMT_I444,
>                   "WebM image format not I420 or I444");
>  
> +    if (mSeekTargetThreshold.isSome()) {

there's a () operator, so the isSome() is optional.

You can do:
if (mSeekTargetThreshold)

I didn't know about it before so I used isSome() myself in many places, but I think not using isSome() looks nicer

::: dom/media/platforms/wmf/WMFMediaDataDecoder.cpp
@@ +260,5 @@
> +{
> +  MOZ_ASSERT(mCallback->OnReaderTaskQueue());
> +  MOZ_DIAGNOSTIC_ASSERT(!mIsShutDown);
> +
> +  int64_t time = aTime.ToMicroseconds();

why use an intermediary int64_t only to convert it back to a TimeUnit ?

please pass to the lambda aTime directly

::: dom/media/platforms/wmf/WMFMediaDataDecoder.h
@@ +36,5 @@
>    virtual HRESULT Output(int64_t aStreamOffset,
>                           RefPtr<MediaData>& aOutput) = 0;
>  
> +  void Flush() {
> +    mSeekTargetThreshold.reset();

I think you need to reset the value *after* flushing. As it's possible that flushing may output frame.

That way you don't unnecessarily create frames that would have been dropped anyway

Comment 9

[Alfredo Yang (:alfredo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a58393084d3

Comment 10

[Alfredo Yang (:alfredo)]

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

Comment 11

[Alfredo Yang (:alfredo)]

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

Comment 12

[Alfredo Yang (:alfredo)]

It causes WMF decoding error after dropping frame in test_HaveMetadataUnbufferedSeek_mp4.html.
Somehow it looks like we can't just drop this frame.

[MediaPDecoder #2]: D/PlatformDecoderModule Dropping video frame which pts is smaller than seek target.
[Child 6368] WARNING: NS_ENSURE_TRUE(SUCCEEDED(hr)) failed: file c:/mozilla-source/mozilla-central/dom/media/platforms/wmf/MFTDecoder.cpp, line 237
[Child 6368] WARNING: WMFVideoMFTManager::Output() unexpected error: file c:/mozilla-source/mozilla-central/dom/media/platforms/wmf/WMFVideoMFTManager.cpp, line 651
[Child 6368] WARNING: WMFMediaDataDecoder failed to output data: file c:/mozilla-source/mozilla-central/dom/media/platforms/wmf/WMFMediaDataDecoder.cpp, line 157

Comment 13

[Jean-Yves Avenard [:jya]]

would you happen to drop the seek target inclusive ? You don't want that to happen. The frame that is exactly the seek target can't be dropped, this will cause failure when we drain the decoder to make sure we have all decoded frames output.

Comment 14

[Alfredo Yang (:alfredo)]

(In reply to Jean-Yves Avenard [:jya] from comment #13)
> would you happen to drop the seek target inclusive ? You don't want that to
> happen. The frame that is exactly the seek target can't be dropped, this
> will cause failure when we drain the decoder to make sure we have all
> decoded frames output.

I guess I found the problem, the buffer of the drop frame needs to be released so it allocates another new buffer in MFTDecoder::Output(). Testing by another try now.

Comment 15

[Alfredo Yang (:alfredo)]

Attachment: skip_decoded_frame_before_seek_target

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84dc32cb1c89

Comment 16

[Alfredo Yang (:alfredo)]

Attachment: skip_decoded_frame_before_seek_target

Rebase.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 18

[Alfredo Yang (:alfredo)]

Attachment: skip_decoded_frame_before_seek_target

Rebase.

Comment 19

[Alfredo Yang (:alfredo)]

Attachment: skip_decoded_frame_before_seek_target

Rebase

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4c5be816f3

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux webm reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d91a79d23b

https://treeherder.mozilla.org/logviewer.html#?job_id=28239576&repo=mozilla-inbound#L7289

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Possibly related as well?
https://treeherder.mozilla.org/logviewer.html#?job_id=28253857&repo=mozilla-inbound

Comment 23

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> Possibly related as well?
> https://treeherder.mozilla.org/logviewer.html#?job_id=28253857&repo=mozilla-
> inbound

Not yours, disregard.

Comment 24

[Alfredo Yang (:alfredo)]

After talking to jya, there are some potential problems:

1. input sample / output sample counts, maybe it could be solved by returning null frame.
2. InternalSeek when switching stream id, MFR needs to set seek threshold again to new decoder.
3. The pts of last frame is not at the end of video. For example, a slide show video. This is the reason of this reftest failure.

Comment 25

[Alfredo Yang (:alfredo)]

Attachment: add_null_data

This is the rough patch to solve the problems we discussed last week.

1. Input/output count problem in MFR. Add NullSample, decoder sends NullSample back to MFR if it is not reach to seek thresholds.

2. InternalSeek when switching stream id. Call SetVideoDecodeThreshold in the lambda of InternalSeek().

3. Always check GetNextRandomAccessPoint before setting threshold to decoder so we can know it is end of stream.

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8755727 [details] [diff] [review]
add_null_data

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

::: dom/media/MediaData.h
@@ +361,5 @@
> +public:
> +  NullData(int64_t aOffset, int64_t aTime, int64_t aDuration, bool aKeyframe)
> +    : MediaData(NULL_DATA, aOffset, aTime, aDuration, 0)
> +  {
> +    mKeyframe = aKeyframe;

do you need to know about the keyframe value or the offset?

::: dom/media/MediaFormatReader.cpp
@@ +1637,5 @@
>    MOZ_ASSERT(OnTaskQueue());
>    LOGV("Video seeked to %lld", aTime.ToMicroseconds());
>    mVideo.mSeekRequest.Complete();
>  
> +  SetVideoDecodeThreshold(mPendingSeekTime.ref());

please move the MOZ_ASSERT(mPendingSeekTime.isSome()) above this line.

But I think this is the wrong place to set the video decode threshold. A decoder may not exist at the time OnVideoSeekCompleted is called, or more likely, after the seek is complete and following the first demux, the resolution would have changed which would cause the decoder to be tear down; the new decoder will not know about the threshold then.

I think you need to set the seek threshold after the decoder has been initialised in MediaFormatReader::EnsureDecoderInitialized.

@@ +1658,5 @@
> +MediaFormatReader::SetVideoDecodeThreshold(media::TimeUnit aTime)
> +{
> +  MOZ_ASSERT(OnTaskQueue());
> +
> +  if (!mVideo.mDecoder) {

make it return early if !HasVideo() you could also have SetVideoDecodeThreshold just use mVideo.mOriginalSeekTarget.GetTime()

@@ +1662,5 @@
> +  if (!mVideo.mDecoder) {
> +    return;
> +  }
> +
> +  media::TimeUnit keyframe;

remove media:: we have using mozilla::media; at the top of this file

@@ +1663,5 @@
> +    return;
> +  }
> +
> +  media::TimeUnit keyframe;
> +  if (NS_FAILED(mVideo.mTrackDemuxer->GetNextRandomAccessPoint(&keyframe))) {

This won't work with some demuxers as the keyframe position is only calculated after the first demux, not right after the seek.

If you call SetVideoDecodeThreshold in EnsureDecoderInitialized however, it should be fine as it's always called after the first demux

::: dom/media/MediaFormatReader.h
@@ +174,5 @@
>    void DrainComplete(TrackType aTrack);
>  
>    bool ShouldSkip(bool aSkipToNextKeyframe, media::TimeUnit aTimeThreshold);
>  
> +  void SetVideoDecodeThreshold(media::TimeUnit aTime);

const media::TimeUnit& ; no need to pass by value (and yes, some other function prototypes should be changed in MediaFormatReader)

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8755727 [details] [diff] [review]
add_null_data

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

::: dom/media/MediaFormatReader.cpp
@@ +1637,5 @@
>    MOZ_ASSERT(OnTaskQueue());
>    LOGV("Video seeked to %lld", aTime.ToMicroseconds());
>    mVideo.mSeekRequest.Complete();
>  
> +  SetVideoDecodeThreshold(mPendingSeekTime.ref());

Actually, to be clearer, you need to set the threshold there *and* after the decoder is being initialised.

Comment 28

[Alfredo Yang (:alfredo)]

Attachment: Bug 1257107: discard decoded data if its pts is smaller than seek time.

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

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8756759 [details]
Bug 1257107: discard decoded data if its pts is smaller than seek time.

https://reviewboard.mozilla.org/r/55390/#review52324

::: dom/media/MediaData.h:368
(Diff revision 1)
> +    : MediaData(NULL_DATA, aOffset, aTime, aDuration, 0)
> +  {}
> +
> +  static const Type sType = NULL_DATA;
> +
> +protected:

I don't believe you need to define a destructor here.

::: dom/media/MediaFormatReader.cpp:1150
(Diff revision 1)
>      }
>    }
>  
> +  while (decoder.mOutput.Length() && decoder.mOutput[0]->mType == MediaData::NULL_DATA) {
> +    LOGV("Dropping null data. Time: %lld", decoder.mOutput[0]->mTime);
> +    decoder.mOutput.RemoveElementAt(0);

you need to decrement decoder.mSizeOfQueue here.

::: dom/media/MediaFormatReader.cpp:1266
(Diff revision 1)
>  
>    LOG("Resolved data promise for %s [%lld, %lld]", TrackTypeToStr(aTrack),
>        aData->mTime, aData->GetEndTime());
>  
>    if (aTrack == TrackInfo::kAudioTrack) {
> -    if (aData->mType != MediaData::RAW_DATA) {
> +    if (aData->mType == MediaData::AUDIO_DATA) {

I don't think you need this as you should ever reach ReturnOutput if the type is NULL_DATA: it must have been dropped before.

In fact I would leave this unchanged and have a MOZ_DIAGNOSTIC_ASSERT at the start of ReturnOutput that aData->mType != MediaData::NULL_DATA

::: dom/media/MediaFormatReader.cpp:1700
(Diff revision 1)
>  void
> +MediaFormatReader::SetVideoDecodeThreshold()
> +{
> +  MOZ_ASSERT(OnTaskQueue());
> +
> +  if (!HasVideo() || !mOriginalSeekTarget.IsValid()) {

This will only work for the plain seeking.

For internal seeking mOriginalSeekTarget would not be the value you want.

Also mOriginalSeekTarget will always be valid once there's a first seek. So whenever a new decoder will be created (like when resolution changes), you will end up setting the threshold too : so you must test that you are actually seeking (you can test this MediaFormatReader::IsSeeking())

As we're now handling two different seeking conditions: normal seeking and internal seek; you have to differentiate between the two.

So something like:
mVideo.mDecoder.SetSeekThreshold( mVideo.mTimeThreshold.isSome() ? mVideo.mTimeThreshold.ref().Time() : mOriginalSeekTarget.GetTime());

something like that

::: dom/media/MediaFormatReader.cpp:1711
(Diff revision 1)
> +    return;
> +  }
> +
> +  // If the key frame is invalid/infinite, it means the target position is
> +  // closing to end of stream. We don't want to skip any frame at this point.
> +  if (mVideo.mDecoder && keyframe.IsValid() && !keyframe.IsInfinite()) {

This will always be true when doing an internal seek. We enter internal seeking with MSE when there's a gap, and we don't have a keyframe in the future. so you can't test for this condition if SetVideoDecodeThreshold is to be used with internal seeking.

::: dom/media/platforms/apple/AppleVDADecoder.h:138
(Diff revision 1)
>    Atomic<uint32_t> mInputIncoming;
>    Atomic<bool> mIsShutDown;
>    const bool mUseSoftwareImages;
>    const bool mIs106;
>  
> -  // Protects mReorderQueue.
> +  // Protects mReorderQueue and mSeekTargetThreshold.

I don't believe you need a monitor to protect mSeekTargetThreshold, simply because SetSeekThreshold will only ever be called at the creation of the decoder or after it's been flushed.

so MediaDataDecoder::Input() hasn't been called yet.

There can't be a race accessing mSeekTargetThreshold

::: dom/media/platforms/apple/AppleVDADecoder.cpp:336
(Diff revision 1)
>    if (!aImage) {
>      // Image was dropped by decoder.
>      return NS_OK;
>    }
>  
> +  bool UseNullSample = false;

useNullSample ; first letter is to be lowercase

Comment 30

[Alfredo Yang (:alfredo)]

https://reviewboard.mozilla.org/r/55390/#review52324

> This will always be true when doing an internal seek. We enter internal seeking with MSE when there's a gap, and we don't have a keyframe in the future. so you can't test for this condition if SetVideoDecodeThreshold is to be used with internal seeking.

So we need to check mWaitingForData?

> I don't believe you need a monitor to protect mSeekTargetThreshold, simply because SetSeekThreshold will only ever be called at the creation of the decoder or after it's been flushed.
> 
> so MediaDataDecoder::Input() hasn't been called yet.
> 
> There can't be a race accessing mSeekTargetThreshold

If I get your point correctly, you mean SetSeekThreshold should be called after Flush or before calling Input(), right?

Comment 31

[Jean-Yves Avenard [:jya]]

(In reply to Alfredo Yang (:alfredo) from comment #30)
> https://reviewboard.mozilla.org/r/55390/#review52324
> 
> > This will always be true when doing an internal seek. We enter internal seeking with MSE when there's a gap, and we don't have a keyframe in the future. so you can't test for this condition if SetVideoDecodeThreshold is to be used with internal seeking.
> 
> So we need to check mWaitingForData?

no..

We are either in normal seek mode, or internal seek mode; never both.

So it's either IsSeeking() that is true, or mVideo.mTimeThreshold that is set.

> If I get your point correctly, you mean SetSeekThreshold should be called
> after Flush or before calling Input(), right?

it is already that way.. There's always a flush prior a seek. and you call SetSeekThreshold right after seeking the demuxer.

Comment 32

[Alfredo Yang (:alfredo)]

Comment on attachment 8756759 [details]
Bug 1257107: discard decoded data if its pts is smaller than seek time.

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

Comment 33

[Jean-Yves Avenard [:jya]]

Comment on attachment 8756759 [details]
Bug 1257107: discard decoded data if its pts is smaller than seek time.

https://reviewboard.mozilla.org/r/55390/#review52882

Could please split this patch into two? one with just the MediaFormatReader changes and the other for the VDA decoder implementation. It's two different scopes really

::: dom/media/MediaFormatReader.cpp:1701
(Diff revisions 1 - 2)
>  
>  void
>  MediaFormatReader::SetVideoDecodeThreshold()
>  {
>    MOZ_ASSERT(OnTaskQueue());
>  

Can you please add a comment that if IsSeeking() is true, then video seek must have completed already.

In having said that, GetNextRandomAccessPoint(&keyframe) will only succeed for webm after a seek.

The MP4 demuxer or the MSE demuxer only set the next keyframe once GetSample is called.

I think it would be better to only call SetVideoDecodeThreshold in OnVideoDemuxCompleted rather than once the video seek complete.

Either do that now, or open a follow up bug.

::: dom/media/MediaFormatReader.cpp:1714
(Diff revisions 1 - 2)
> +    threshold = mVideo.mTimeThreshold.ref().Time();
> +  } else if (IsSeeking()) {
> -  TimeUnit keyframe;
> +    TimeUnit keyframe;
> -  if (NS_FAILED(mVideo.mTrackDemuxer->GetNextRandomAccessPoint(&keyframe))) {
> +    if (NS_FAILED(mVideo.mTrackDemuxer->GetNextRandomAccessPoint(&keyframe))) {
> -    return;
> +      return;
> -  }
> +    }

please return early if neither of those cases are true.

Comment 34

[Alfredo Yang (:alfredo)]

https://reviewboard.mozilla.org/r/55390/#review52882

Sure, thanks for review!

> Can you please add a comment that if IsSeeking() is true, then video seek must have completed already.
> 
> In having said that, GetNextRandomAccessPoint(&keyframe) will only succeed for webm after a seek.
> 
> The MP4 demuxer or the MSE demuxer only set the next keyframe once GetSample is called.
> 
> I think it would be better to only call SetVideoDecodeThreshold in OnVideoDemuxCompleted rather than once the video seek complete.
> 
> Either do that now, or open a follow up bug.

I'll open a follow up bug.

Comment 35

[Alfredo Yang (:alfredo)]

Attachment: Bug 1257107: set video seek thresohld on mac PDM.

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

Comment 36

[Alfredo Yang (:alfredo)]

Comment on attachment 8756759 [details]
Bug 1257107: discard decoded data if its pts is smaller than seek time.

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

Comment 37

[Jean-Yves Avenard [:jya]]

Comment on attachment 8758136 [details]
Bug 1257107: set video seek thresohld on mac PDM.

https://reviewboard.mozilla.org/r/56502/#review53490

Comment 38

[Alfredo Yang (:alfredo)]

Comment on attachment 8756759 [details]
Bug 1257107: discard decoded data if its pts is smaller than seek time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55390/diff/3-4/

Comment 39

[Alfredo Yang (:alfredo)]

Comment on attachment 8758136 [details]
Bug 1257107: set video seek thresohld on mac PDM.

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

Comment 40

[Alfredo Yang (:alfredo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dfb1d19ec81

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5133065436e2
discard decoded data if its pts is smaller than seek time. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd290590f9e
set video seek thresohld on mac PDM. r=jya

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5133065436e2
https://hg.mozilla.org/mozilla-central/rev/8dd290590f9e