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

RESOLVED FIXED in Firefox 50

Status

()

P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: cpearce, Assigned: ayang)

Tracking

(Blocks: 5 bugs)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Assignee: nobody → dglastonbury
(Reporter)

Updated

3 years ago
Priority: -- → P2
(Reporter)

Comment 1

3 years ago
Someone on Taipei team is welcome to take this on.
Assignee: dglastonbury → nobody
(Assignee)

Updated

3 years ago
Assignee: nobody → ayang
(Assignee)

Comment 2

3 years ago
Created attachment 8737113 [details] [diff] [review]
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.
Attachment #8737113 - Flags: feedback?(cpearce)
(Reporter)

Comment 3

3 years ago
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?
Attachment #8737113 - Flags: feedback?(cpearce) → feedback?(jyavenard)
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.
Attachment #8737113 - Flags: feedback?(jyavenard)
(Assignee)

Comment 5

2 years ago
Created attachment 8745892 [details] [diff] [review]
skip_decoded_frame_before_seek_target
Attachment #8737113 - Attachment is obsolete: true
Attachment #8745892 - Flags: review?(jyavenard)
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?
Attachment #8745892 - Flags: review?(jyavenard)
(Assignee)

Comment 7

2 years ago
Created attachment 8746985 [details] [diff] [review]
skip_decoded_frame_before_seek_target
Attachment #8745892 - Attachment is obsolete: true
Attachment #8746985 - Flags: review?(jyavenard)
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
Attachment #8746985 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 12

2 years ago
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
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.
(Assignee)

Comment 14

2 years ago
(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.
(Assignee)

Comment 15

2 years ago
Created attachment 8753214 [details] [diff] [review]
skip_decoded_frame_before_seek_target

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84dc32cb1c89
Attachment #8746985 - Attachment is obsolete: true
Attachment #8753214 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 16

2 years ago
Created attachment 8753218 [details] [diff] [review]
skip_decoded_frame_before_seek_target

Rebase.
Attachment #8753214 - Attachment is obsolete: true
Attachment #8753218 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
(Assignee)

Comment 18

2 years ago
Created attachment 8753664 [details] [diff] [review]
skip_decoded_frame_before_seek_target

Rebase.
Attachment #8753218 - Attachment is obsolete: true
Attachment #8753664 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 19

2 years ago
Created attachment 8754231 [details] [diff] [review]
skip_decoded_frame_before_seek_target

Rebase
Attachment #8753664 - Attachment is obsolete: true
Attachment #8754231 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(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.
(Assignee)

Comment 24

2 years ago
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.
(Assignee)

Comment 25

2 years ago
Created attachment 8755727 [details] [diff] [review]
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.
Attachment #8755727 - Flags: feedback?(jyavenard)
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)
Attachment #8755727 - Flags: feedback?(jyavenard)
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.
See Also: → bug 1275538
(Assignee)

Comment 28

2 years ago
Created attachment 8756759 [details]
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/
Attachment #8756759 - Flags: review?(jyavenard)
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
Attachment #8756759 - Flags: review?(jyavenard)
(Assignee)

Comment 30

2 years ago
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?
(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.
(Assignee)

Comment 32

2 years ago
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/
Attachment #8756759 - Flags: review?(jyavenard)
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.
Attachment #8756759 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 34

2 years ago
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.
(Assignee)

Comment 35

2 years ago
Created attachment 8758136 [details]
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/
Attachment #8756759 - Attachment description: MozReview Request: Bug 1257107: discard decoded data if its pts is smaller than seek time. r?jya → MozReview Request: Bug 1257107: discard decoded data if its pts is smaller than seek time. r=jya
Attachment #8758136 - Flags: review?(jyavenard)
(Assignee)

Comment 36

2 years ago
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/
(Assignee)

Updated

2 years ago
Blocks: 1276881
Comment on attachment 8758136 [details]
Bug 1257107: set video seek thresohld on mac PDM.

https://reviewboard.mozilla.org/r/56502/#review53490
Attachment #8758136 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 38

2 years ago
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/
Attachment #8756759 - Attachment description: MozReview Request: Bug 1257107: discard decoded data if its pts is smaller than seek time. r=jya → Bug 1257107: discard decoded data if its pts is smaller than seek time.
Attachment #8758136 - Attachment description: MozReview Request: Bug 1257107: set video seek thresohld on mac PDM. r=jya → Bug 1257107: set video seek thresohld on mac PDM.
(Assignee)

Comment 39

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8754231 - Attachment is obsolete: true

Comment 41

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 42

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5133065436e2
https://hg.mozilla.org/mozilla-central/rev/8dd290590f9e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Updated

2 years ago
Blocks: 1280036
(Assignee)

Updated

2 years ago
Blocks: 1281115
(Assignee)

Updated

2 years ago
Blocks: 1282722
Depends on: 1286454
Depends on: 1286466
Depends on: 1300292
You need to log in before you can comment on or make changes to this bug.