MediaDecoderStateMachine waits for decoded audio/video before firing loadedmetadata.

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ajones, Assigned: jya)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: the order to apply is: 1065827, 1092979 and 1093654)

Attachments

(7 attachments, 31 obsolete attachments)

1.62 KB, patch
Details | Diff | Splinter Review
45.15 KB, patch
Details | Diff | Splinter Review
2.58 KB, patch
Details | Diff | Splinter Review
1.94 KB, patch
Details | Diff | Splinter Review
1.93 KB, patch
Details | Diff | Splinter Review
881 bytes, patch
Details | Diff | Splinter Review
1.28 KB, patch
eflores
: review+
Details | Diff | Splinter Review
The loadedmetadata event should be fired as soon as the initialisation segment has been appended. At the moment it waits until the first data fragment has been appended.

The call to Decoder(kAudio) in MP4Reader::ReadMetaData for HE-AAC support blocks reading the metadata. There is an additional a problem with the demuxer looking for the first moof box.
Bug 1065850 should fix part of the issue. The remaining bit is http://mxr.mozilla.org/mozilla-central/source/content/media/fmp4/MP4Reader.cpp#399

The issue is that in HE-AAC the sample rate is reported as half of what it really is. So for a 48K samples/sec it gets reported as 24K. We don't know the correct rate until we decode the first sample.

One way to deal with this would be to delay the creation of the audio stream. Another would be to always configure the audio to 44.1K or 48K and do software resampling.
I think I would look at delaying the creation of the audio stream before adding resampling to the pipeline.

The risk with delaying audio stream creation is adding latency to the start of playback, but the cost of resampling is noticeable cpu use, ~5% of a CPU.  Perhaps that resampling cost will be paid somewhere anyway, but perhaps the hardware has a more efficient way to do this.
(Assignee)

Comment 3

5 years ago
There's not many proper methods to resample, and it's all done via floating point calculations. It can be extremely CPU intensive, especially on processors without FPU (ARM in particular)

5% only CPU usage indicates to me a resampling without a very impressive SNR.
Having said that, as indicated by kinetic, resampling is something that will have to be done regardless somewhere
We should probably create the audio stream when the first successfully decoded audio sample arrives in the MediaDecoderStateMachine (in the DECODER_STATE_DECODING_METADATA case of MediaDecoderStateMachine::OnAudioDecoded()). This means we will not delay creating the audio stream until when play() is called for the first time, as creating audio streams can be slow which could cause playback start latency.

Also note that MediaDecoderStateMachine::DecodeMetadata() requests one audio and one video sample from the Reader, and we don't exit reading metadata state until those decoded samples come in. So we won't dispatch the loadedmetadata event until we've decoded the first samples.

So in MediaDecoderStateMachine::DecodeMetadata() after we've added the MediaQueue pop listeners, we could try just calling FinishDecodeMetadata() (like we do in the mScheduler->IsRealTime() path, but mScheduler->IsRealTime() should not return true for MSE, as that has other effects undesirable for MSE). That would mean metadata loading won't wait for the first samples to be decoded. We will however assume a stream start time of 0, we may need to do something else here.
(Assignee)

Updated

5 years ago
Assignee: nobody → jyavenard
(Assignee)

Comment 5

5 years ago
Test to reproduce issue:
http://people.mozilla.org/~jyavenard/tests/mse_mp4/paper-init.html?chunks=1&eos_chunk=5&eos_delay=-1&delay_chunk=0&start=-1

the loadedmetadata event is never fired as the MP4Reader is waiting for more data.
(Assignee)

Comment 6

5 years ago
Preliminary version. This has ultimately the same behaviour as earlier as loadedmetadata is always issued *after* reading the first video/audio packet
(Assignee)

Comment 7

5 years ago
Attempt without introducing a new state in the state machine. This was an attempt to reduce potential problems due to too many disruptive changes. Unfortunately, the behaviour is identical to the previous one: deadlock and seek errors
(Assignee)

Comment 8

5 years ago
Initial implementation using a new state FIRSTFRAME_LOADING. content/media/test/test_seek-4.html mochitest will time-out and I can't figure out why.
Attachment #8510226 - Flags: review?(jwwang)
(Assignee)

Updated

5 years ago
Attachment #8510018 - Attachment is obsolete: true
Comment on attachment 8510226 [details] [diff] [review]
Do not always wait to decode frames to emit loadedmetadata

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

::: content/media/AbstractMediaDecoder.h
@@ +175,5 @@
>  };
>  
> +class FirstFrameLoadedEventRunner : public nsRunnable
> +{
> +  private:

We don't indent access modifiers which also mean the indentation of MetadataEventRunner is wrong.

@@ +214,5 @@
> +    return NS_OK;
> +  }
> +
> +  // The ownership is transferred to MediaDecoder.
> +  MediaInfo* mInfo;

I am really having a hard time in tracking the ownership of these members. I think it is time to fix them once and for all by using nsAutoPtr or UniquePtr.

::: content/media/MediaDecoder.cpp
@@ +731,5 @@
> +  DECODER_LOG("FirstFrameLoaded, channels=%u rate=%u hasAudio=%d hasVideo=%d",
> +              aInfo->mAudio.mChannels, aInfo->mAudio.mRate,
> +              aInfo->HasAudio(), aInfo->HasVideo());
> +
> +  if (mPlayState == PLAY_STATE_LOADING && mIsDormant && !mIsExitingDormant) {

This is already handled in MediaDecoder::MetadataLoaded(). Do we also need to handle it here? Btw, you need locks to access the members.

@@ +734,5 @@
> +
> +  if (mPlayState == PLAY_STATE_LOADING && mIsDormant && !mIsExitingDormant) {
> +    return;
> +  }
> +  DurationChanged();

What is it for?

::: content/media/MediaDecoderReader.h
@@ +105,5 @@
> +  // Returns true if MediaInfo got updated or false otherwise.
> +  // ReadUpdatedMetadata will always be called once ReadMetadata has succeeded.
> +  virtual bool ReadUpdatedMetadata(MediaInfo* aInfo)
> +  {
> +    (void)aInfo;

We have mozilla::unused_t.

::: content/media/MediaDecoderStateMachine.cpp
@@ +1982,5 @@
> +    // Inform the element that we've loaded the metadata.
> +    EnqueueLoadedMetadataEvent();
> +  }
> +
> +  SetState(DECODER_STATE_DECODING_FIRSTFRAME_LOADING);

We need to check if |mState == DECODER_STATE_DECODING_METADATA| before proceeding to next state. Otherwise we could overwrite |mState == DECODER_STATE_SHUTDOWN|.

@@ +2012,5 @@
> +    DecodeError();
> +  }
> +}
> +
> +nsresult MediaDecoderStateMachine::FirstFrameLoading()

The function name should be a verb. I would prefer DecodeFirstFrame or LoadFirstFrame.

@@ +2101,5 @@
>    }
>  
> +  // Get potentially updated metadata
> +  {
> +    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());

We shouldn't need to release lock since we are just reading cached values.

@@ +2103,5 @@
> +  // Get potentially updated metadata
> +  {
> +    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> +    MediaInfo info;
> +    if (mReader->ReadUpdatedMetadata(&info)) {

Why don't we just update mInfo anyway?

@@ +2112,4 @@
>    nsAutoPtr<MediaInfo> info(new MediaInfo());
>    *info = mInfo;
> +  nsCOMPtr<nsIRunnable> event;
> +  if (!mGotDurationFromMetaData) {

We should check |mGotDurationFromMetaData = (GetDuration() != -1)| again.

@@ +2126,5 @@
> +  }
> +  NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> +
> +  if (mState == DECODER_STATE_DECODING_FIRSTFRAME_LOADING) {
> +    DECODER_LOG("Changed state from DECODING_FIRSTFRAME_LOADING to DECODING");

MediaDecoderStateMachine::SetState() will print this message for you.
Attachment #8510226 - Flags: review?(jwwang)
(Assignee)

Comment 10

5 years ago
(In reply to JW Wang [:jwwang] from comment #9)
> Comment on attachment 8510226 [details] [diff] [review]
> Do not always wait to decode frames to emit loadedmetadata
> 
> Review of attachment 8510226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/AbstractMediaDecoder.h
> @@ +175,5 @@
> >  };
> >  
> > +class FirstFrameLoadedEventRunner : public nsRunnable
> > +{
> > +  private:
> 
> We don't indent access modifiers which also mean the indentation of
> MetadataEventRunner is wrong.

Oversight ... XCode does so automatically during copy/paste :(

> 
> @@ +214,5 @@
> > +    return NS_OK;
> > +  }
> > +
> > +  // The ownership is transferred to MediaDecoder.
> > +  MediaInfo* mInfo;
> 
> I am really having a hard time in tracking the ownership of these members. I
> think it is time to fix them once and for all by using nsAutoPtr or
> UniquePtr.

I don't believe this is the scope of this ticket or this patch... another bug can be opened for that

> 
> ::: content/media/MediaDecoder.cpp
> @@ +731,5 @@
> > +  DECODER_LOG("FirstFrameLoaded, channels=%u rate=%u hasAudio=%d hasVideo=%d",
> > +              aInfo->mAudio.mChannels, aInfo->mAudio.mRate,
> > +              aInfo->HasAudio(), aInfo->HasVideo());
> > +
> > +  if (mPlayState == PLAY_STATE_LOADING && mIsDormant && !mIsExitingDormant) {
> 
> This is already handled in MediaDecoder::MetadataLoaded(). Do we also need
> to handle it here? Btw, you need locks to access the members.

Yes, because MediaDecoder::MetadataLoaded and MediaDecoder::FirstFrameLoaded can be called right after the other. If it does nothing in the first, it should also do nothing in the second.
This is done to preserve the existing behaviour

> 
> @@ +734,5 @@
> > +
> > +  if (mPlayState == PLAY_STATE_LOADING && mIsDormant && !mIsExitingDormant) {
> > +    return;
> > +  }
> > +  DurationChanged();
> 
> What is it for?

Duration can be updated once we've decoded the first frame. As such, the media decoder needs to know about those changes.
Detecting change in duration between what's reported in the demuxer and after decoding a frame is the core reason on why we were waiting on the first frame before issuing loadedmetadata

> 
> ::: content/media/MediaDecoderReader.h
> @@ +105,5 @@
> > +  // Returns true if MediaInfo got updated or false otherwise.
> > +  // ReadUpdatedMetadata will always be called once ReadMetadata has succeeded.
> > +  virtual bool ReadUpdatedMetadata(MediaInfo* aInfo)
> > +  {
> > +    (void)aInfo;
> 
> We have mozilla::unused_t.

ok

> 
> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +1982,5 @@
> > +    // Inform the element that we've loaded the metadata.
> > +    EnqueueLoadedMetadataEvent();
> > +  }
> > +
> > +  SetState(DECODER_STATE_DECODING_FIRSTFRAME_LOADING);
> 
> We need to check if |mState == DECODER_STATE_DECODING_METADATA| before
> proceeding to next state. Otherwise we could overwrite |mState ==
> DECODER_STATE_SHUTDOWN|.

ok

> 
> @@ +2012,5 @@
> > +    DecodeError();
> > +  }
> > +}
> > +
> > +nsresult MediaDecoderStateMachine::FirstFrameLoading()
> 
> The function name should be a verb. I would prefer DecodeFirstFrame or
> LoadFirstFrame.

Question of personal preferences really. To me, loading indicates far more explicitly that we are in the middle of doing it.
As opposed to FirstFrameLoaded which happens once we're done.

> 
> @@ +2101,5 @@
> >    }
> >  
> > +  // Get potentially updated metadata
> > +  {
> > +    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> 
> We shouldn't need to release lock since we are just reading cached values.

We can't assume what the demuxer is simply going to return a cache value. Sure, this is how I've implemented the MP4Reader's one, that doesn't mean in the future it will be done that way. Especially as there's already a hint that it would be required to handle some 5.1 HE-AAC content.

> 
> @@ +2103,5 @@
> > +  // Get potentially updated metadata
> > +  {
> > +    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> > +    MediaInfo info;
> > +    if (mReader->ReadUpdatedMetadata(&info)) {
> 
> Why don't we just update mInfo anyway?

I found this way more elegant. It removes restrictions on the implementation of ReadUpdatedMetadata to never modify the parameter unless it succeeded. Something the MP4Reader::ReadMetadata doesn't always do.

> 
> @@ +2112,4 @@
> >    nsAutoPtr<MediaInfo> info(new MediaInfo());
> >    *info = mInfo;
> > +  nsCOMPtr<nsIRunnable> event;
> > +  if (!mGotDurationFromMetaData) {
> 
> We should check |mGotDurationFromMetaData = (GetDuration() != -1)| again.

no. that would cause MediaDecoder::LoadedMetadata never to be fired should the duration not be determined.
I also use mGutDurationFromMetadata as a hint if we need to be in DECODING_METADATA or DECODING_FIRSTFRAME_LOADING state elsewhere in the state machine.

Metadata now in the context of the state machine only means the metadata as determined by the demuxer.
We can easily check the duration by calling GetDuration(). That we determined the duration from the demuxer is an information that we need to determine the order in which we fire events.

> 
> @@ +2126,5 @@
> > +  }
> > +  NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> > +
> > +  if (mState == DECODER_STATE_DECODING_FIRSTFRAME_LOADING) {
> > +    DECODER_LOG("Changed state from DECODING_FIRSTFRAME_LOADING to DECODING");
> 
> MediaDecoderStateMachine::SetState() will print this message for you.

It's a left over over debugging stuff... we remove that.

Thanks for your comments!

Now need to find a way to prevent multiple concurrent seeking event not to be fired when we start decoding data in the middle of a seek (which cause test_seek-4 and others to timeout)
(Assignee)

Comment 11

5 years ago
(In reply to JW Wang [:jwwang] from comment #9)
> Comment on attachment 8510226 [details] [diff] [review]
> Do not always wait to decode frames to emit loadedmetadata
> 
> Review of attachment 8510226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/AbstractMediaDecoder.h
> @@ +175,5 @@
> >  };
> >  
> > +class FirstFrameLoadedEventRunner : public nsRunnable
> > +{
> > +  private:
> 
> We don't indent access modifiers which also mean the indentation of
> MetadataEventRunner is wrong.

actually, the indentation is identical to the exiting code (FirstFrameLoadedEventRunner is just a copy of MetadataEventRunner).

So that makes change of indentation out of the scope of this patch.
Comment on attachment 8510226 [details] [diff] [review]
Do not always wait to decode frames to emit loadedmetadata

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

::: content/media/MediaDecoder.cpp
@@ +734,5 @@
> +
> +  if (mPlayState == PLAY_STATE_LOADING && mIsDormant && !mIsExitingDormant) {
> +    return;
> +  }
> +  DurationChanged();

Then it should be the job of the state machine to notify the duration change as in MediaDecoderStateMachine::UpdateEstimatedDuration().

::: content/media/MediaDecoderStateMachine.cpp
@@ +1536,5 @@
>    // in that case MediaDecoder shouldn't be calling us.
>    NS_ASSERTION(mState != DECODER_STATE_SEEKING,
>                 "We shouldn't already be seeking");
> +  NS_ASSERTION(mState > (mGotDurationFromMetaData ? DECODER_STATE_DECODING_METADATA : DECODER_STATE_DECODING_FIRSTFRAME_LOADING),
> +               "We should have got duration already");

Why checking mGotDurationFromMetaData? Shouldn't we only seek after starting decoding?

@@ +2103,5 @@
> +  // Get potentially updated metadata
> +  {
> +    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> +    MediaInfo info;
> +    if (mReader->ReadUpdatedMetadata(&info)) {

It appears to me you want mInfo to be up to date. I will prefer |mReader->ReadUpdatedMetadata(&mInfo)| to let the reader decide to update mInfo if necessary. And you can save the if statement then.
(Assignee)

Comment 13

5 years ago
(In reply to JW Wang [:jwwang] from comment #12)
> Comment on attachment 8510226 [details] [diff] [review]
> Do not always wait to decode frames to emit loadedmetadata
> 
> Review of attachment 8510226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoder.cpp
> @@ +734,5 @@
> > +
> > +  if (mPlayState == PLAY_STATE_LOADING && mIsDormant && !mIsExitingDormant) {
> > +    return;
> > +  }
> > +  DurationChanged();
> 
> Then it should be the job of the state machine to notify the duration change
> as in MediaDecoderStateMachine::UpdateEstimatedDuration().

MediaDecoder::LoadedMetadata changed the duration directly, which is why I followed the existing logic..

Will do.

> 
> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +1536,5 @@
> >    // in that case MediaDecoder shouldn't be calling us.
> >    NS_ASSERTION(mState != DECODER_STATE_SEEKING,
> >                 "We shouldn't already be seeking");
> > +  NS_ASSERTION(mState > (mGotDurationFromMetaData ? DECODER_STATE_DECODING_METADATA : DECODER_STATE_DECODING_FIRSTFRAME_LOADING),
> > +               "We should have got duration already");
> 
> Why checking mGotDurationFromMetaData? Shouldn't we only seek after starting
> decoding?

it would be much easier yes...

But in theory, there's no reason to as loadedmetadata tells that we have the duration, and you're allowed to start seeking.

The other issue is that we could be in a situation very similar to the current one (which we are trying to fix).
Say we have a DASH player waiting for loadedmetadata event to be fired, and immediately start seeking (like for resuming playback position).

If it then only starts feeding data once it received the "seeking" event, it would be blocking as the statemachine is itself waiting for data to issue seeking event

> 
> @@ +2103,5 @@
> > +  // Get potentially updated metadata
> > +  {
> > +    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> > +    MediaInfo info;
> > +    if (mReader->ReadUpdatedMetadata(&info)) {
> 
> It appears to me you want mInfo to be up to date. I will prefer
> |mReader->ReadUpdatedMetadata(&mInfo)| to let the reader decide to update
> mInfo if necessary. And you can save the if statement then.

The idea is actually to have to reader to tell the statemachine if the MediaInfo has changed, and take appropriate action when doing so.
MediaReader::ReadMetadata actually returns a nsresult, but I couldn't find what nsresult would mean: there's no error, but the MediaInfo haven't changed since last time.

I haven't implemented that at this stage, but left the door opened to do so. I much prefer to have a design that allows future extension without changing the API, than only implement things when wanting them.

old habit I guess.
(Assignee)

Comment 14

5 years ago
k17e: do we need to bother about the situation I mentioned in comment 13?
I'm encountering more and more issues in the MediaDecoderStateMachine that is proving to do seek before we have decoded data a rather complicated task
Flags: needinfo?(ajones)
(In reply to Jean-Yves Avenard [:jya] from comment #13)

> MediaDecoder::LoadedMetadata changed the duration directly, which is why I
> followed the existing logic..
MediaDecoder::MetadataLoaded() reads the duration from MediaDecoderStateMachine::GetDuration() so that they have a consistent view in the duration. However, it is the job of the state machine to detect duration changes and notify the decoder to fire 'durationchange' if necessary.

> The idea is actually to have to reader to tell the statemachine if the
> MediaInfo has changed, and take appropriate action when doing so.
> MediaReader::ReadMetadata actually returns a nsresult, but I couldn't find
> what nsresult would mean: there's no error, but the MediaInfo haven't
> changed since last time.
> 
> I haven't implemented that at this stage, but left the door opened to do so.
> I much prefer to have a design that allows future extension without changing
> the API, than only implement things when wanting them.
As far as API concerned, can we have the old good MediaDecoderReader::ReadMetadata() to return cached values whenever possible to suit your needs?
(Assignee)

Comment 16

5 years ago
(In reply to JW Wang [:jwwang] from comment #15)
> (In reply to Jean-Yves Avenard [:jya] from comment #13)
> 
> > MediaDecoder::LoadedMetadata changed the duration directly, which is why I
> > followed the existing logic..
> MediaDecoder::MetadataLoaded() reads the duration from
> MediaDecoderStateMachine::GetDuration() so that they have a consistent view
> in the duration. However, it is the job of the state machine to detect
> duration changes and notify the decoder to fire 'durationchange' if
> necessary.

ok will do.

> As far as API concerned, can we have the old good
> MediaDecoderReader::ReadMetadata() to return cached values whenever possible
> to suit your needs?

that was my first idea, and yeah it would be nice. But there are a lot of MediaDecoderReader::ReadMetadata implementation to check on, and I don't have all the platforms to verify. The chances of introducing a regression are big ; adding a new method to get an updated value is *much* easier and the chances of a regression are very low...
(In reply to Jean-Yves Avenard [:jya] from comment #14)
> k17e: do we need to bother about the situation I mentioned in comment 13?
> I'm encountering more and more issues in the MediaDecoderStateMachine that
> is proving to do seek before we have decoded data a rather complicated task

I don't know.
Flags: needinfo?(ajones)
(Assignee)

Comment 18

5 years ago
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #17)
> (In reply to Jean-Yves Avenard [:jya] from comment #14)
> > k17e: do we need to bother about the situation I mentioned in comment 13?
> > I'm encountering more and more issues in the MediaDecoderStateMachine that
> > is proving to do seek before we have decoded data a rather complicated task
> 
> I don't know.

I think the issue is moot anyway. I can't see how we can properly calculate duration and current playback position without demuxing the first frame to get it's time stamp
(Assignee)

Updated

5 years ago
Attachment #8510049 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Update with comments, only start seeking once we have loaded the first frame
(Assignee)

Updated

5 years ago
Attachment #8510226 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Prevent potential issue where MDSM::SeekCompleted is run while MDSM::DecodeSeek has been queued
Attachment #8511831 - Flags: review?(jwwang)
Can you describe the test case or scenario which this patch tries to fix?
(Assignee)

Comment 22

5 years ago
MediaSourceDecoder starts in PAUSED mode, which causes issues later on as we won't enable queued seeks.
Attachment #8511897 - Flags: review?(cpearce)
(Assignee)

Comment 23

5 years ago
(In reply to JW Wang [:jwwang] from comment #21)
> Can you describe the test case or scenario which this patch tries to fix?

When MDSM::DecodeSeek is called (started from MediaDecoder::Seek); under most circumstances, when another Seek is issued immediately, the new call to MDSM::DecodeSeek is queued before the first MDSM::SeekCompleted has the chance to run. In that case, all is good.

From time to time however, a new frame is received by the time machine, that adds a small delay and when the new seek occurs and MDSK::DecodeSeek is queued once again, the first MDSM::DecodeSeek has already been called and queued MDSM::SeekCompleted. But the time the 2nd MDSM::SeekDecode is then called, we aren't in DECODER_STATE_SEEKING state anymore, but back in DECODER_STATE_DECODING and MDSM::DecodeSeek will abort early, leading to the "seeking" event to never be fired. And the mochi tests performing multiple seeks will timeout. (such as content/media/test/test_seek-4.html)

It happens in around 20% of the time on my machine. And all the time when we weren't waiting for the first frame to be loaded.
Comment on attachment 8511831 [details] [diff] [review]
Prevent racing condition when multiple seeks are issued

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1709,5 @@
>    mDropVideoUntilNextDiscontinuity = HasVideo();
>  
>    RefPtr<nsIRunnable> task(
>      NS_NewRunnableMethod(this, &MediaDecoderStateMachine::DecodeSeek));
> +  mSeekDecodeQueued = true;

You can tell that a seek is in progress because mCurrentSeekTarget should be valid. Is that insufficient for tracking what you're tracking here? Note mCurrentSeekTarget is set in EnqueueDecodeSeekTask() and reset in SeekCompleted() (when the seek succeeds).
(Assignee)

Comment 25

5 years ago
(In reply to Chris Pearce (:cpearce) from comment #24)
> Comment on attachment 8511831 [details] [diff] [review]
> Prevent racing condition when multiple seeks are issued
> 
> Review of attachment 8511831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +1709,5 @@
> >    mDropVideoUntilNextDiscontinuity = HasVideo();
> >  
> >    RefPtr<nsIRunnable> task(
> >      NS_NewRunnableMethod(this, &MediaDecoderStateMachine::DecodeSeek));
> > +  mSeekDecodeQueued = true;
> 
> You can tell that a seek is in progress because mCurrentSeekTarget should be
> valid. Is that insufficient for tracking what you're tracking here? Note
> mCurrentSeekTarget is set in EnqueueDecodeSeekTask() and reset in
> SeekCompleted() (when the seek succeeds).

We need to know that information upon running SeekCompleted
mCurrentSeekTarget would be valid regardless of how many seeks you have queued. So it isn't sufficient information to know if we've had a call to EnqueueDecodeSeekTask in between the queue of SeekCompleted and the time SeekCompleted gets to actually run
Attachment #8511897 - Flags: review?(cpearce) → review+
Comment on attachment 8511831 [details] [diff] [review]
Prevent racing condition when multiple seeks are issued

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1709,5 @@
>    mDropVideoUntilNextDiscontinuity = HasVideo();
>  
>    RefPtr<nsIRunnable> task(
>      NS_NewRunnableMethod(this, &MediaDecoderStateMachine::DecodeSeek));
> +  mSeekDecodeQueued = true;

The 2nd seek in the 'seeking' handler should always abort the 1st seek per spec. This approach fails because SeekCompleted() (from 1st seek) might complete before the state machine thread has a chance to run EnqueueDecodeSeekTask() which is enqueued by Seek() in the main thread.
Attachment #8511831 - Flags: review?(jwwang)
(Assignee)

Comment 27

5 years ago
(In reply to JW Wang [:jwwang] from comment #26)
> Comment on attachment 8511831 [details] [diff] [review]
 
> The 2nd seek in the 'seeking' handler should always abort the 1st seek per
> spec. This approach fails because SeekCompleted() (from 1st seek) might
> complete before the state machine thread has a chance to run
> EnqueueDecodeSeekTask() which is enqueued by Seek() in the main thread.

Exactly...
which is exactly what this patch is achieving by preventing SeekCompleted from running (it aborts early). Another option would be to remove from the mDecodeTaskQueue the queued call to SeekCompleted.
No, it still won't work because SeekCompleted might be in action and it is too late to remove it. We need a way to synchronize execution between 3 threads (main, state machine, and decoding threads).

MediaDecoderStateMachineScheduler::FreezeScheduling was invented to provide synchronization between state machine and decoding threads. But it is still insufficient in this case as long as main thread is involved.
(Assignee)

Updated

5 years ago
See Also: → 1090009
(Assignee)

Updated

5 years ago
Attachment #8511831 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
Moved that particular problem to its own bug. You can take it.
(Assignee)

Comment 30

5 years ago
Now have state machine queue seeks received while in DECODING_FIRSTFRAME, so we are in seeking mode as soon as currentTime was modified. Required to pass test_seek-1. Passes all tests mochitest-1 now.
Attachment #8512512 - Flags: review?(jwwang)
(Assignee)

Updated

5 years ago
Attachment #8511830 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8511897 - Attachment is obsolete: true
(Assignee)

Comment 32

5 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ab053bc6f1f5

I have the feeling that there's another race that causes intermittent failures with slower platforms like B2G
(Assignee)

Updated

5 years ago
Attachment #8512512 - Attachment is obsolete: true
Attachment #8512512 - Flags: review?(jwwang)
(Assignee)

Comment 33

5 years ago
Updated MediaDecoderReader::ReadUpdatedMetadata as per review
Attachment #8512530 - Flags: review?(jwwang)
(In reply to Jean-Yves Avenard [:jya] from comment #23)
> From time to time however, a new frame is received by the time machine, that
> adds a small delay and when the new seek occurs and MDSK::DecodeSeek is
> queued once again, the first MDSM::DecodeSeek has already been called and
> queued MDSM::SeekCompleted. But the time the 2nd MDSM::SeekDecode is then
> called, we aren't in DECODER_STATE_SEEKING state anymore, but back in
> DECODER_STATE_DECODING and MDSM::DecodeSeek will abort early, leading to the
> "seeking" event to never be fired. And the mochi tests performing multiple
> seeks will timeout. (such as content/media/test/test_seek-4.html)
> 
> It happens in around 20% of the time on my machine. And all the time when we
> weren't waiting for the first frame to be loaded.

Looking at this again. The 2nd seek in the 'seeking' handler should see |mPlayState == PLAY_STATE_SEEKING| in MediaDecoder::Seek() and will not enqueue MDSM::DecodeSeek. Did I miss something?
Just noticed bug 1089480 comment 6 which seems related to this bug. If we always have 0 as the start time in MSE case, does that also mean we don't even have to decode the 1st frame to get start time in the MSE case?
Comment on attachment 8512530 [details] [diff] [review]
Do not always wait to decode frames to emit loadedmetadata

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +923,5 @@
>  
>      case DECODER_STATE_BUFFERING:
>      case DECODER_STATE_DECODING: {
>        Push(video.forget());
> +      if (mVideoDecodeStartTime.IsNull()) {

How is it possible not having a decoded sample in OnVideoDecoded()?

@@ +1545,5 @@
>    // in that case MediaDecoder shouldn't be calling us.
>    NS_ASSERTION(mState != DECODER_STATE_SEEKING,
>                 "We shouldn't already be seeking");
> +  NS_ASSERTION(mState > DECODER_STATE_DECODING_METADATA,
> +               "We should have got duration already");

The description seems wrong. Changes are we don't have duration until the 1st frame is decoded. (ref: mGotDurationFromMetaData)

@@ +1550,5 @@
> +
> +  if (mState <= DECODER_STATE_DECODING_FIRSTFRAME) {
> +    DECODER_LOG("Seek() Not Enough Data to continue at this stage, queuing seek");
> +    mSeekTarget = aTarget;
> +    ScheduleStateMachine();

No need to re-schedule for there is no state change.

@@ +1565,5 @@
> +  AssertCurrentThreadInMonitor();
> +
> +  if (mState == DECODER_STATE_SHUTDOWN) {
> +    return;
> +  }

Also assert |mState >= DECODER_STATE_DECODING|.

@@ +2505,5 @@
>      }
>  
> +    case DECODER_STATE_DECODING_FIRSTFRAME: {
> +      // Ensure we have a decode thread to start decoding.
> +      return EnqueueFirstFrameTask();

If the state machine is scheduled again, you will enqueue the task twice.

::: dom/media/MediaDecoderStateMachine.h
@@ +401,5 @@
>  
>    MediaQueue<AudioData>& AudioQueue() { return mAudioQueue; }
>    MediaQueue<VideoData>& VideoQueue() { return mVideoQueue; }
>  
> +  nsresult FinishFirstFrame();

maybe "FinishDecodeFirstFrame"

@@ +533,5 @@
> +
> +  // Dispatches a task to the decode task queue to begin decoding content.
> +  // This is threadsafe and can be called on any thread.
> +  // The decoder monitor must be held.
> +  nsresult EnqueueFirstFrameTask();

maybe "EnqueueDecodeFirstFrameTask"

@@ +594,5 @@
>    void CallDecodeMetadata();
>  
> +  // Initiate first content decoding. Called on the decode thread.
> +  // The decoder monitor must be held with exactly one lock count.
> +  nsresult FirstFrame();

maybe "DecodeFirstFrame"

@@ +597,5 @@
> +  // The decoder monitor must be held with exactly one lock count.
> +  nsresult FirstFrame();
> +
> +  // Wraps the call to FirstFrameLoading(), signals a DecodeError() on failure.
> +  void CallFirstFrame();

maybe "CallDecodeFirstFrame"

@@ +601,5 @@
> +  void CallFirstFrame();
> +
> +  // Checks whether we're finished decoding first audio and/or video packets,
> +  // and switches to DECODING state if so.
> +  void MaybeFinishFirstFrame();

MaybeFinishDecodeFirstFrame
Attachment #8512530 - Flags: review?(jwwang)
(Assignee)

Comment 37

5 years ago
(In reply to JW Wang [:jwwang] from comment #36)
> Comment on attachment 8512530 [details] [diff] [review]
> Do not always wait to decode frames to emit loadedmetadata
> 
> Review of attachment 8512530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +923,5 @@
> >  
> >      case DECODER_STATE_BUFFERING:
> >      case DECODER_STATE_DECODING: {
> >        Push(video.forget());
> > +      if (mVideoDecodeStartTime.IsNull()) {
> 
> How is it possible not having a decoded sample in OnVideoDecoded()?

You tell me ! :)

Actually, the comment is wrong, we have a decoded sample, but mVideoDecodeStartTime is null.

when this happens it causes an assert in operator-:

21:33:45  WARNING -  PROCESS-CRASH | /tests/content/media/mediasource/test/test_SplitAppendDelay.html | application crashed [@ mozilla::TimeStamp::operator-(mozilla::TimeStamp const&) const]

Check: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jyavenard@mozilla.com-5056bb5dc832/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-1-bm117-tests1-linux64-build395.txt.gz

21:33:45     INFO -  Crash dump filename: /tmp/tmpWoloqp.mozrunner/minidumps/284bd493-8795-d516-13bac169-3c064688.dmp
21:33:45     INFO -  Operating system: Linux
21:33:45     INFO -                    0.0.0 Linux 3.2.0-23-generic #36-Ubuntu SMP Tue Apr 10 20:39:51 UTC 2012 x86_64
21:33:45     INFO -  CPU: amd64
21:33:45     INFO -       family 6 model 62 stepping 4
21:33:45     INFO -       1 CPU
21:33:45     INFO -  Crash reason:  SIGSEGV
21:33:45     INFO -  Crash address: 0x0
21:33:45     INFO -  Thread 45 (crashed)
21:33:45     INFO -   0  libxul.so!mozilla::TimeStamp::operator-(mozilla::TimeStamp const&) const [TimeStamp.h:5056bb5dc832 : 438 + 0x17]
21:33:45     INFO -      rbx = 0x00007fa32e30f000   r12 = 0x00007fa32e3ccb50
21:33:45     INFO -      r13 = 0x00007fa32e2bb400   r14 = 0x0000000000000000
21:33:45     INFO -      r15 = 0x0000000000000000   rip = 0x00007fa3684434b4
21:33:45     INFO -      rsp = 0x00007fa3513abb40   rbp = 0x00007fa3513abb40
21:33:45     INFO -      Found by: given as instruction pointer in context
21:33:45     INFO -   1  libxul.so!mozilla::MediaDecoderStateMachine::OnVideoDecoded(mozilla::VideoData*) [MediaDecoderStateMachine.cpp:5056bb5dc832 : 929 + 0x15]
21:33:45     INFO -      rbx = 0x00007fa32e30f000   r12 = 0x00007fa32e3ccb50
21:33:45     INFO -      r13 = 0x00007fa32e2bb400   r14 = 0x0000000000000000
21:33:45     INFO -      r15 = 0x0000000000000000   rip = 0x00007fa36969ca4e
21:33:45     INFO -      rsp = 0x00007fa3513abb50   rbp = 0x00007fa3513abb90
21:33:45     INFO -      Found by: call frame info
21:33:45     INFO -   2  libxul.so!mozilla::MediaDataDecodedListener<mozilla::MediaDecoderStateMachine>::DeliverVideoTask::Run() [MediaDataDecodedListener.h:5056bb5dc832 : 137 + 0x4]
21:33:45     INFO -      rbx = 0x00007fa32e227240   r12 = 0x00007fa32e22a790
21:33:45     INFO -      r13 = 0x00007fa32e2bb400   r14 = 0x0000000000000000
21:33:45     INFO -      r15 = 0x0000000000000000   rip = 0x00007fa36969cbf7
21:33:45     INFO -      rsp = 0x00007fa3513abba0   rbp = 0x00007fa3513abba0
21:33:45     INFO -      Found by: call frame info
21:33:45     INFO -   3  libxul.so!mozilla::MediaTaskQueue::Runner::Run() [MediaTaskQueue.cpp:5056bb5dc832 : 194 + 0x5]

I actually had a TODO item on that one:
// TODO: Find why this is required. We shouldn't get here if that was the case

Will action the other points.. (wondering how many times I'm going to be asked to rename those functions ! ;) )
(Assignee)

Comment 38

5 years ago
(In reply to JW Wang [:jwwang] from comment #36)

> @@ +1545,5 @@
> >    // in that case MediaDecoder shouldn't be calling us.
> >    NS_ASSERTION(mState != DECODER_STATE_SEEKING,
> >                 "We shouldn't already be seeking");
> > +  NS_ASSERTION(mState > DECODER_STATE_DECODING_METADATA,
> > +               "We should have got duration already");
> 
> The description seems wrong. Changes are we don't have duration until the
> 1st frame is decoded. (ref: mGotDurationFromMetaData)

yes, but then we wouldn't be here as loadedmetadata wouldn't have been fired ; we would still be in STATE_PLAY_LOADING and no Seek would be done by MediaDecoder.

> > +    case DECODER_STATE_DECODING_FIRSTFRAME: {
> > +      // Ensure we have a decode thread to start decoding.
> > +      return EnqueueFirstFrameTask();
> 
> If the state machine is scheduled again, you will enqueue the task twice.

great find !
What do you suggest to prevent this ?
another boolean member to mark that we've already queued the task, and therefore should do nothing ?

Wouldn't there be a problem with the other states as well?
likeL
case DECODER_STATE_SEEKING:

that would queue more than one DecodeSeek
(Assignee)

Comment 39

5 years ago
(In reply to JW Wang [:jwwang] from comment #35)
> Just noticed bug 1089480 comment 6 which seems related to this bug. If we
> always have 0 as the start time in MSE case, does that also mean we don't
> even have to decode the 1st frame to get start time in the MSE case?

Was told about this this morning...

The issue is to do with some MSE streams (YouTube) where it's possible to tell YouTube to seek from the URL and have the first frame received be in the middle of the stream.

I'm not sure how we can handle that with the current state machine. It expects the mStartTime to be the time of the first frame and all duration and position within the stream, use when seeking are dependent on that mStartTime to be valid ; otherwise we could get negative playback position (and this is the current and last crash I'm seeing: we get negative times)...
Matt Woodrow has been looking into the problem, but from the description given to me this morning, it doesn't strike me as being a valid approach with the current code. I can't see how this one can be reliably resolved.
(In reply to Jean-Yves Avenard [:jya] from comment #38)
> yes, but then we wouldn't be here as loadedmetadata wouldn't have been fired
> ; we would still be in STATE_PLAY_LOADING and no Seek would be done by
> MediaDecoder.
It appears to me attachment 8512513 [details] [diff] [review] should fix this problem. MediaDecoder will not exit STATE_PLAY_LOADING until the first frame is loaded. So we don't have to change the code of MediaDecoderStateMachine::Seek which will only be called after 1st frame loaded.

> great find !
> What do you suggest to prevent this ?
> another boolean member to mark that we've already queued the task, and
> therefore should do nothing ?
EnqueueDecodeMetadataTask used to be called in DECODER_STATE_DECODING_METADATA and suffered from the same problem. At the time, a boolean flag was invented to prevent enqueue twice. Later on, I introduced a new state DECODER_STATE_DECODING_NONE and was able to remove the boolean flag by enqueuing the task during state transition.

> Wouldn't there be a problem with the other states as well?
> likeL
> case DECODER_STATE_SEEKING:
> 
> that would queue more than one DecodeSeek
It is handled by MediaDecoder to ensure no more than one DecodeSeek is queued.
(In reply to Jean-Yves Avenard [:jya] from comment #37)
> Actually, the comment is wrong, we have a decoded sample, but
> mVideoDecodeStartTime is null.

The possibility I can think of is:
1. mReader->RequestVideoData is called at [1] without setting |mVideoDecodeStartTime|
2. somehow mState changes to DECODER_STATE_BUFFERING or DECODER_STATE_DECODING
3. the first decoded video frame returns to OnVideoDecoded() and hits the assertion.


[1] http://hg.mozilla.org/mozilla-central/file/53d84829b2b8/dom/media/MediaDecoderStateMachine.cpp#l2000
Chrome's code just blindly assumes that the start time for a MSE source is 0.

The patch I landed for bug 1089480 makes us do the same, so we always set mStartTime = 0, regardless of what the time on the first frame we get is.

I'm not sure if there's anything else we could do here, we have no guarantee of ever getting the first frame, so we can't rely on the timing contained within.
If we just want to get the timestamp of first frame for start time, it is doable to get it from demuxer instead of decoding the first frame.
(Assignee)

Updated

5 years ago
Depends on: 1090900
(Assignee)

Comment 44

5 years ago
wait for loadeddata event to run test. Now that loadedmetadata can be fired before we have data, the HTMLVideoElement.readyState will be HAVE_METADATA. According to the spec 'if the image argument is an HTMLVideoElement object whose readyState attribute is either HAVE_NOTHING or HAVE_METADATA, then the implementation must return without drawing anything.' , so attempting to draw will not cause an error
Attachment #8514085 - Flags: review?(padenot)
(Assignee)

Updated

5 years ago
Attachment #8514085 - Attachment is obsolete: true
Attachment #8514085 - Flags: review?(padenot)
(Assignee)

Comment 45

5 years ago
updated function names
Attachment #8514094 - Flags: review?(padenot)
(Assignee)

Comment 46

5 years ago
Ensure DecodeFirstFrame is never queued more than once. Always run queued seeks from main thread to ensure some synchronization
Attachment #8514228 - Flags: review?(jwwang)
(Assignee)

Updated

5 years ago
Attachment #8512530 - Attachment is obsolete: true
(Assignee)

Comment 47

5 years ago
Fix various leaks related to passing MediaInfo from state machine to media decoder
(Assignee)

Comment 49

5 years ago
Comment on attachment 8514228 [details] [diff] [review]
Do not always wait to decode frames to emit loadedmetadata

The removal of Invalidate() in MediaDecoder::MetadataLoaded causes all the reftest to fail
Attachment #8514228 - Attachment is obsolete: true
Attachment #8514228 - Flags: review?(jwwang)
(Assignee)

Comment 51

5 years ago
rebase
(Assignee)

Updated

5 years ago
Attachment #8514231 - Attachment is obsolete: true
(Assignee)

Comment 52

5 years ago
fix reftests
Attachment #8514280 - Flags: review?(padenot)
(Assignee)

Updated

5 years ago
Attachment #8514277 - Flags: review?(jwwang)
(Assignee)

Updated

5 years ago
Attachment #8514278 - Flags: review?(jwwang)
Comment on attachment 8514278 [details] [diff] [review]
Fix various memory leaks in MediaDecoders

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

::: dom/media/AbstractMediaDecoder.h
@@ +177,3 @@
>  };
>  
> +class FirstFrameLoadedEventRunner : public MetadataEventRunner

I don't appreciate the idea to subclass in order just to reuse the data store (mInfo and mTags). I would prefer composition or private inheritance then.

@@ +191,3 @@
>  };
>  
> +class MetadataUpdatedEventRunner : public MetadataEventRunner

Ditto.

::: dom/media/MediaDecoder.cpp
@@ +714,5 @@
>    if (mDuration == -1) {
>      SetInfinite(true);
>    }
>  
> +  mInfo = info;

I would prefer to say info.forget() to make memory ownership transfer obvious and you will notice not to use info anymore.

::: dom/media/mediasource/SourceBufferDecoder.cpp
@@ +89,5 @@
>    return false;
>  }
>  
>  void
>  SourceBufferDecoder::MetadataLoaded(MediaInfo* aInfo, MetadataTags* aTags)

Why not changing the signature to (nsAutoPtr<MediaInfo> aInfo, nsAutoPtr<MetadataTags> aTags)? The memory ownership is clear then.
Attachment #8514278 - Flags: review?(jwwang)
Comment on attachment 8514277 [details] [diff] [review]
Do not always wait to decode frames to emit loadedmetadata

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1228,5 @@
>  static const char* const gMachineStateStr[] = {
>    "NONE",
>    "DECODING_METADATA",
>    "WAIT_FOR_RESOURCES",
> +  "FIRSTFRAME",

DECODING_FIRSTFRAME

@@ +1543,5 @@
>                 "We shouldn't already be seeking");
> +  NS_ASSERTION(mState > DECODER_STATE_DECODING_METADATA,
> +               "We should have got duration already");
> +
> +  if (mState <= DECODER_STATE_DECODING_FIRSTFRAME) {

Maybe it is easier to check |mPlayState != PLAY_STATE_SEEKING && mPlayState != PLAY_STATE_LOADING| at http://hg.mozilla.org/mozilla-central/file/675913ddbb55/dom/media/MediaDecoder.cpp#l640.

It appears to me we shouldn't issue seek command to the state machine before LOADING is finished. And it is much easier that you don't have to deal with thread issues.

@@ +1568,5 @@
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  if (!mQueuedSeekTarget.IsValid()) {
> +    return;
> +  }
> +  StartSeek(mQueuedSeekTarget);

You can reset mQueuedSeekTarget in this function so that you don't need to reset it in multiple return path in StartSeek()

@@ +1579,5 @@
> +  AssertCurrentThreadInMonitor();
> +
> +  MOZ_ASSERT(mState >= DECODER_STATE_DECODING);
> +
> +  if (mState == DECODER_STATE_SHUTDOWN ) {

space after DECODER_STATE_SHUTDOWN.
Attachment #8514277 - Flags: review?(jwwang) → review+
(Assignee)

Comment 56

5 years ago
(In reply to JW Wang [:jwwang] from comment #55)
> Maybe it is easier to check |mPlayState != PLAY_STATE_SEEKING && mPlayState
> != PLAY_STATE_LOADING| at
> http://hg.mozilla.org/mozilla-central/file/675913ddbb55/dom/media/
> MediaDecoder.cpp#l640.
> 
> It appears to me we shouldn't issue seek command to the state machine before
> LOADING is finished. And it is much easier that you don't have to deal with
> thread issues.

It is much easier, but that wouldn't work. It's one of the first implementation I had, but this caused several mochitest to fail (such as dom/media/test/test_seek-1.html).
In particular, when the loadedmetadata event is fired, they change the currentTime and immediately read the seeking attribute and test that it is now true.
Not setting PLAY_STATE_SEEKING immediately and only set it once MediaDecoder::FirstFrameLoaded is called, would delay setting the seeking attribute to true.

seeking being asynchronous, we can delay its execution, but seeking state must be set immediately.
(Assignee)

Comment 57

5 years ago
(In reply to JW Wang [:jwwang] from comment #55)

> @@ +1568,5 @@
> > +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> > +  if (!mQueuedSeekTarget.IsValid()) {
> > +    return;
> > +  }
> > +  StartSeek(mQueuedSeekTarget);
> 
> You can reset mQueuedSeekTarget in this function so that you don't need to
> reset it in multiple return path in StartSeek()

We can, but StartSeek can be called from MediaDecoder::Seek and should also reset any pending mQueuedSeekTarget.
Resulting in the exact same number of call to mQueuedSeekTarget.Reset(). I felt it was better to have it all in the same function.
(Assignee)

Comment 58

5 years ago
(In reply to JW Wang [:jwwang] from comment #55)
> Maybe it is easier to check |mPlayState != PLAY_STATE_SEEKING && mPlayState
> != PLAY_STATE_LOADING| at
> http://hg.mozilla.org/mozilla-central/file/675913ddbb55/dom/media/
> MediaDecoder.cpp#l640.
> 
> It appears to me we shouldn't issue seek command to the state machine before
> LOADING is finished. And it is much easier that you don't have to deal with
> thread issues.

It is much easier, but that wouldn't work. It's one of the first implementation I had, but this caused several mochitest to fail (such as dom/media/test/test_seek-1.html).
In particular, when the loadedmetadata event is fired, they change the currentTime and immediately read the seeking attribute and test that it is now true.
Not setting PLAY_STATE_SEEKING immediately and only set it once MediaDecoder::FirstFrameLoaded is called, would delay setting the seeking attribute to true.

seeking being asynchronous, we can delay its execution, but seeking state must be set immediately.

It could all be done in MediaDecoder, we set PLAY_STATE_SEEKING immediately, but delay the call to MediaDecoderStateMachine::Seek ; the changes required would be similar in size as the one in the state machine. not sure it's worth the bother
(Assignee)

Updated

5 years ago
Attachment #8514277 - Attachment is obsolete: true
(Assignee)

Comment 60

5 years ago
Carrying r+
Attachment #8514825 - Flags: review?(jwwang)
(Assignee)

Updated

5 years ago
Attachment #8514278 - Attachment is obsolete: true
(Assignee)

Comment 61

5 years ago
there was another potential leak in the Ogg reader, which I addressed in the new patch
(Assignee)

Updated

5 years ago
Attachment #8514785 - Flags: review+
Comment on attachment 8514825 [details] [diff] [review]
Fix various memory leaks in MediaDecoders

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

::: dom/media/AbstractMediaDecoder.h
@@ +163,5 @@
> +  nsAutoPtr<MediaInfo>  mInfo;
> +  nsAutoPtr<MetadataTags> mTags;
> +};
> +
> +class MetadataEventRunner : public nsRunnable, public MetadataContainer

Use private MetadataContainer

@@ +166,5 @@
> +
> +class MetadataEventRunner : public nsRunnable, public MetadataContainer
> +{
> +public:
> +  MetadataEventRunner(AbstractMediaDecoder* aDecoder, MediaInfo* aInfo, MetadataTags* aTags)

nsAutoPtr<MediaInfo> and nsAutoPtr<MetadataTags> for the paramenter types. So the user of MetadataEventRunner is aware of the memory transfer.

@@ +196,3 @@
>  {
> +public:
> +  MetadataUpdatedEventRunner(AbstractMediaDecoder* aDecoder, MediaInfo* aInfo, MetadataTags* aTags)

Ditto for parameter types.

::: dom/media/MediaDecoder.cpp
@@ +720,5 @@
>    if (mOwner) {
>      // Make sure the element and the frame (if any) are told about
>      // our new size.
>      Invalidate();
> +    mOwner->MetadataLoaded(mInfo, aTags.forget());

MediaDecoderOwner::MetadataLoaded doesn't take the memory.

::: dom/media/MediaDecoderOwner.h
@@ +50,5 @@
>  
>    // Called by the video decoder object, on the main thread,
>    // when it has read the metadata containing video dimensions,
>    // etc.
> +  // Must take ownership of MetadataTags aTags argument.

The description is wrong. HTMLMediaElement::MetadataLoaded doesn't take the ownership.

::: dom/media/ogg/OggReader.cpp
@@ +845,5 @@
>        nsAutoPtr<MediaInfo> info(new MediaInfo());
>        *info = mInfo;
>        ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>        mDecoder->QueueMetadata((mDecodedAudioFrames * USECS_PER_S) / mInfo.mAudio.mRate,
> +                              info, tags);

info.forget(), tags.forget()
Attachment #8514825 - Flags: review?(jwwang)
(Assignee)

Comment 63

5 years ago
(In reply to JW Wang [:jwwang] from comment #62)
> Comment on attachment 8514825 [details] [diff] [review]

> nsAutoPtr<MediaInfo> and nsAutoPtr<MetadataTags> for the paramenter types.
> So the user of MetadataEventRunner is aware of the memory transfer.

ok.

> 
> @@ +196,3 @@
> >  {
> > +public:
> > +  MetadataUpdatedEventRunner(AbstractMediaDecoder* aDecoder, MediaInfo* aInfo, MetadataTags* aTags)
> 
> Ditto for parameter types.
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +720,5 @@
> >    if (mOwner) {
> >      // Make sure the element and the frame (if any) are told about
> >      // our new size.
> >      Invalidate();
> > +    mOwner->MetadataLoaded(mInfo, aTags.forget());
> 
> MediaDecoderOwner::MetadataLoaded doesn't take the memory.

???
MediaDecoderOwner::MetadataLoaded will take the the tags (it's also in the original documentation of AbstractMediaDecoder.h: "// The ownership is transferred to its owning element."
)
... not doing so would either cause a leak. or a crash as you would a double freed pointer.

Now there could be another leak there in the default MediaDecoderOwner::MetadataLoaded implementation, but that's outside the scope of this patch.. HTMLMediaElement certainly takes ownership of it (it copies the value to mTags which is a nsAutoPtr<const MetadataTags>

> 
> ::: dom/media/MediaDecoderOwner.h
> @@ +50,5 @@
> >  
> >    // Called by the video decoder object, on the main thread,
> >    // when it has read the metadata containing video dimensions,
> >    // etc.
> > +  // Must take ownership of MetadataTags aTags argument.
> 
> The description is wrong. HTMLMediaElement::MetadataLoaded doesn't take the
> ownership.

yes it does. see above. That you could think so is the primary reason of why I saw the need to clarify the documentation.

> 
> ::: dom/media/ogg/OggReader.cpp
> @@ +845,5 @@
> >        nsAutoPtr<MediaInfo> info(new MediaInfo());
> >        *info = mInfo;
> >        ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> >        mDecoder->QueueMetadata((mDecodedAudioFrames * USECS_PER_S) / mInfo.mAudio.mRate,
> > +                              info, tags);
> 
> info.forget(), tags.forget()

What's the point of using nsAutoPtr if it's not to use any of its implicit methods?
The documentation of nsAutoPtr is rather explicit, operator= transfers ownership: "assign by transferring ownership from another smart pointer."

Despite doing as you suggest, while it would likely be more explicit for a casual reader, wouldn't even compile as there's no explicit constructor from T* to nsAutoPtr<T>. So you can't do info.forget() or tags.forget() as MediaDecoder::QueueMetadata expects now a nsAutoPtr

We seem to have a different understanding of what reviews are for, and the meaning of r- :(
Comment on attachment 8514825 [details] [diff] [review]
Fix various memory leaks in MediaDecoders

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

::: dom/media/MediaDecoderOwner.h
@@ +50,5 @@
>  
>    // Called by the video decoder object, on the main thread,
>    // when it has read the metadata containing video dimensions,
>    // etc.
> +  // Must take ownership of MetadataTags aTags argument.

Looking at the code of HTMLMediaElement::MetadataLoaded(), I find aInfo is not transferred but aTags is. Again, this is confusing.
(Assignee)

Comment 65

5 years ago
(In reply to JW Wang [:jwwang] from comment #64)
> Comment on attachment 8514825 [details] [diff] [review]
> Fix various memory leaks in MediaDecoders
> 
> Review of attachment 8514825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderOwner.h
> @@ +50,5 @@
> >  
> >    // Called by the video decoder object, on the main thread,
> >    // when it has read the metadata containing video dimensions,
> >    // etc.
> > +  // Must take ownership of MetadataTags aTags argument.
> 
> Looking at the code of HTMLMediaElement::MetadataLoaded(), I find aInfo is
> not transferred but aTags is. Again, this is confusing.

Yes.
I will create a another patch that will attempt to clear up this confusion..

But I don't want to delay this bug any longer. It's imperative that we have this for MSE. So probably will create another ticket.
(Assignee)

Comment 66

5 years ago
I should had, that really R- a patch because you disagree with the original code rather sucks. I didn't make those function prototypes nor designed the original API: I'm only working with those, trying to find a good compromise between something that will work and keeping the changes to a minimum.

That MediaDecoderOwner::MetadataLoaded takes ownership of the MetadataTags but only keep a reference to the MediaInfo is not my doing.
It is not the job of this patch to fix that, only to fix existing leaks.

IMHO the scope of a patch should be kept to a minimum: two functional changes == two patches
(In reply to Jean-Yves Avenard [:jya] from comment #65)
> Yes.
> I will create a another patch that will attempt to clear up this confusion..
> 
> But I don't want to delay this bug any longer. It's imperative that we have
> this for MSE. So probably will create another ticket.

Ok, I will accept the patch as long as it is leak free. We can fix the coding style and readibility later.
(In reply to Jean-Yves Avenard [:jya] from comment #66)
> I should had, that really R- a patch because you disagree with the original
> code rather sucks. I didn't make those function prototypes nor designed the
> original API: I'm only working with those, trying to find a good compromise
> between something that will work and keeping the changes to a minimum.
> 
> That MediaDecoderOwner::MetadataLoaded takes ownership of the MetadataTags
> but only keep a reference to the MediaInfo is not my doing.
> It is not the job of this patch to fix that, only to fix existing leaks.
> 
> IMHO the scope of a patch should be kept to a minimum: two functional
> changes == two patches

Code review is for the code quality which include space, indentation, readbility, robustness, design pattern and more. I would prefer to fix style or potential problems as well as the bug altogeter where the changes are involved. However, according to the emergency of this bug, I should put focus on the correctness and delay style problems to another bug.
(In reply to Jean-Yves Avenard [:jya] from comment #63)
> What's the point of using nsAutoPtr if it's not to use any of its implicit
> methods?
> The documentation of nsAutoPtr is rather explicit, operator= transfers
> ownership: "assign by transferring ownership from another smart pointer."
Yes, but it is less obvious and you have to look at the document or the function signature to realize whether the memory will be transferred. For example:

void foo(T*);

nsAutoPtr<T> p;
foo(p); // is the memory transferred?
foo(p.forget()); // the memory is transferred!
 
> Despite doing as you suggest, while it would likely be more explicit for a
> casual reader, wouldn't even compile as there's no explicit constructor from
> T* to nsAutoPtr<T>. So you can't do info.forget() or tags.forget() as
> MediaDecoder::QueueMetadata expects now a nsAutoPtr
I see. You can say nsAutoPtr<T> p(nullptr) but not nsAutoPtr<T> p = nullptr. Sorry for the wrong comment.
(In reply to JW Wang [:jwwang] from comment #69)
> foo(p); // is the memory transferred?
> foo(p.forget()); // the memory is transferred!

That works for nsRefPtr.

For nsAutoPtr, I think the preferred notation, if not moving to UniquePtr<T>, which is beyond the scope of this bug, is now

void foo(nsAutoPtr<MetadataTags> aTags);

foo(Move(p));

foo(p); still works as a concession, but is less clear, as you say.

https://groups.google.com/forum/#!topic/mozilla.dev.platform/Ji3T5kipJzA
(Assignee)

Comment 71

5 years ago
(In reply to JW Wang [:jwwang] from comment #69)
> (In reply to Jean-Yves Avenard [:jya] from comment #63)
> > What's the point of using nsAutoPtr if it's not to use any of its implicit
> > methods?
> > The documentation of nsAutoPtr is rather explicit, operator= transfers
> > ownership: "assign by transferring ownership from another smart pointer."
> Yes, but it is less obvious and you have to look at the document or the
> function signature to realize whether the memory will be transferred. For
> example:
> 
> void foo(T*);
> 
> nsAutoPtr<T> p;
> foo(p); // is the memory transferred?
> foo(p.forget()); // the memory is transferred!

The current code is confusing, and that's why its leaking! What I'm surprised about is that it wasn't detecting the leaks before, and reshuffling the order of events suddenly made the leaks appearant. 

The other issues is we start going down the route if changing function prototyping to make it clearer, you're opening pandora box. There are lots of functions in this code that currently are taking a pointer, some of the functions will take ownership of the objects, some won't. And like HTMLMediaElelement::MetadataLoaded keep a reference to one and ownership of the other. How can anyone keep track of what's going on?

As a compromise, and focusing only on the immediate issues at hand, I can change the prototyping of MediaDecoderOwner::MetadataLoaded to nsRefPtr<MediaInfo>, nsAutoPtr<MetadataTags>
Change MediaDecoderStateMachine::QueueMetadata to also take nsAutoPtr because that one two got me confused, and leave it at that for the time being.

>  
> > Despite doing as you suggest, while it would likely be more explicit for a
> > casual reader, wouldn't even compile as there's no explicit constructor from
> > T* to nsAutoPtr<T>. So you can't do info.forget() or tags.forget() as
> > MediaDecoder::QueueMetadata expects now a nsAutoPtr
> I see. You can say nsAutoPtr<T> p(nullptr) but not nsAutoPtr<T> p = nullptr.
> Sorry for the wrong comment.

It has bitten me a few times in the past...
You cant assign a raw pointer to a RefPtr, so you can't make the code explicit without comments like you suggested.

I can add such comments whenever we transfer ownership, for the benefit of the poor soul that will have to work next on this part of the code :)
I had amended the documentation in the header files for this reason.
(Assignee)

Comment 72

5 years ago
Damn iPad autocorrection two should read too, and a few if should read of :)
(Assignee)

Comment 73

5 years ago
Updating prototyping to make ownership more explicit. Some const-ness difficulties as you can't use nsAutoPtr<const T> in the same fashion as const *T
Attachment #8515444 - Flags: review?(jwwang)
(Assignee)

Updated

5 years ago
Attachment #8514825 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8515444 - Attachment is obsolete: true
Attachment #8515444 - Flags: review?(jwwang)
(Assignee)

Comment 75

5 years ago
Add some nsAutoPtr::forget() where we can to make it more explicit that we are transferring ownership and make my reviewer happier :)
Comment on attachment 8515581 [details] [diff] [review]
Fix various memory leaks in MediaDecoders

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

LGTM.
Attachment #8515581 - Flags: review+
(Assignee)

Updated

5 years ago
Blocks: 1092979
(Assignee)

Comment 77

5 years ago
Mochitest: currently broken due to bug 1092996. Changing 318 for 348 will make it work
Attachment #8515920 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 78

5 years ago
Comment on attachment 8515920 [details] [diff] [review]
Add mochitest to ensure loadedmetada is emitted as early as possible

wrong bug#...
Attachment #8515920 - Attachment is obsolete: true
Attachment #8515920 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

5 years ago
Attachment #8514785 - Attachment is obsolete: true
(Assignee)

Comment 80

5 years ago
Changing reviewer as padenot appear to not be available
Attachment #8516302 - Flags: review?(jwwang)
(Assignee)

Updated

5 years ago
Attachment #8514094 - Attachment is obsolete: true
Attachment #8514094 - Flags: review?(padenot)
(Assignee)

Updated

5 years ago
Attachment #8514280 - Attachment is obsolete: true
Attachment #8514280 - Flags: review?(padenot)
(Assignee)

Comment 81

5 years ago
Rebase
(Assignee)

Updated

5 years ago
Attachment #8515581 - Attachment is obsolete: true
(Assignee)

Comment 82

5 years ago
Changing reviewer as padenot appear to not be available
Attachment #8516305 - Flags: review?(jwwang)
Attachment #8516302 - Flags: review?(jwwang) → review+
Comment on attachment 8516305 [details] [diff] [review]
Use onloadeddata event for reftests

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

Does it work on B2G? Since preload='auto' means 'none' on B2G, I wonder somehow it just works on B2G.
Attachment #8516305 - Flags: review?(jwwang) → review+
(Assignee)

Comment 84

5 years ago
(In reply to JW Wang [:jwwang] from comment #83)
> Comment on attachment 8516305 [details] [diff] [review]
> Use onloadeddata event for reftests
> 
> Review of attachment 8516305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does it work on B2G? Since preload='auto' means 'none' on B2G, I wonder
> somehow it just works on B2G.

Reftest on B2G are all greens. And I've run about 10 tries since making that patch.

Actually here, for that test, that patch isn't fully required... 

I have another round of try going, that adds mochitest from bug 1092979, I will paste the url once completed
I see. I guess there is some pref setting changes when running the reftest.
(Assignee)

Comment 86

5 years ago
IRC, most of the reftest are disabled on B2G
(Assignee)

Comment 87

5 years ago
Attempt to fix some failures on windows with reftests. With this change it appears to have fixed them.
Attachment #8516507 - Flags: review?(jwwang)
(Assignee)

Comment 88

5 years ago
Typical failure in reftest:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=158da94bdd5b
"REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/webm-video/clipping-1a.html | image comparison (==), max difference: 254, number of differing pixels: 17200"
Comment on attachment 8516507 [details] [diff] [review]
Invalidate video frame container should resolution changed

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

::: dom/media/MediaDecoder.cpp
@@ +742,5 @@
>    }
>  
> +  if (mInfo->HasVideo() != aInfo->HasVideo() ||
> +      (aInfo->HasVideo() && (aInfo->mVideo.mDisplay != mInfo->mVideo.mDisplay))) {
> +    Invalidate();

I think we can call invalidate anyway as we did in MediaDecoder::MetadataLoaded() since we expect the UI will change after first frame loaded.
Attachment #8516507 - Flags: review?(jwwang) → review+
(Assignee)

Comment 90

5 years ago
(In reply to JW Wang [:jwwang] from comment #89)
> Comment on attachment 8516507 [details] [diff] [review]
> Invalidate video frame container should resolution changed
> 
> Review of attachment 8516507 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +742,5 @@
> >    }
> >  
> > +  if (mInfo->HasVideo() != aInfo->HasVideo() ||
> > +      (aInfo->HasVideo() && (aInfo->mVideo.mDisplay != mInfo->mVideo.mDisplay))) {
> > +    Invalidate();
> 
> I think we can call invalidate anyway as we did in
> MediaDecoder::MetadataLoaded() since we expect the UI will change after
> first frame loaded.

Should I remove the test and call invalidate all the time you think ?
Originally you had told me that Invalidate wasn't required at all..
Does the test fail because the video size not updated or frame not drawn?
(In reply to Jean-Yves Avenard [:jya] from comment #90)
> Should I remove the test and call invalidate all the time you think ?
> Originally you had told me that Invalidate wasn't required at all..

I said Invalidate() in MediaDecoder::MetadataLoaded should be moved to the media element which would read metadata and update media size as needed. I am not sure why we need to call Invalidate() again after 1st frame loaded to pass the reftest.
(Assignee)

Comment 93

5 years ago
(In reply to JW Wang [:jwwang] from comment #91)
> Does the test fail because the video size not updated or frame not drawn?

I can't say. I can't reproduce it locally...

And when I was missing the Invalidate in MediaDecoder::MetadataLoaded, I couldn't explain the failure either nor do I know how to read the result of those reftest.

Now, if the size in the MediaInfo changes, we do not pass it to the MediaDecoderOwner (here HTMLMediaElement) like with do when we call HTMLMediaElement::MetadataLoaded.

Should we pass the MediaInfo there as well and call HTMLMediaElement::UpdateMediaSize with the new size (or call ResetState accordingly)?
It is simpler to just call Invalidate() which will update the media size of the media owner. By the time 1st frame is loaded, the state machine has rendered the frame and Invalidate() should work correctly.
(Assignee)

Comment 95

5 years ago
In the MediaDecoder ?

so do I leave the patch as-is, or call Invalidate() all the time in MediaDecoder::FirstFrameLoaded
Call Invalidate() all the time in MediaDecoder::FirstFrameLoaded. This is similar to we call Invalidate() in MediaDecoder::MetadataLoaded() for now.
(Assignee)

Updated

5 years ago
Blocks: 1093945
(Assignee)

Comment 97

5 years ago
Temporarily disable mediasource-seek-beyond-duration.html webref test
Attachment #8517079 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 98

5 years ago
Carrying r+. Always call Invalidate() in MediaDecoder::FirstFrameLoaded
(Assignee)

Updated

5 years ago
Attachment #8516507 - Attachment is obsolete: true
(Assignee)

Comment 99

5 years ago
Comment on attachment 8517079 [details] [diff] [review]
Temporarily disable mediasource-seek-beyond-duration.html webref test

not sufficient to disable a test :(
Attachment #8517079 - Attachment is obsolete: true
Attachment #8517079 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 101

5 years ago
Comment on attachment 8517170 [details] [diff] [review]
Temporarily disable mediasource-seek-beyond-duration.html webref test

karlt suggested a better approach
Attachment #8517170 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8517218 - Flags: review?(karlt)
The web platform tests seem to be fine in comment 88.

What changed between the different sets of results?

What are the new results?

I'll be able to get back to you faster if you can tell me what is wrong with the tests, requiring disabling the entire tests.
Flags: needinfo?(jyavenard)
(Assignee)

Comment 104

5 years ago
(In reply to Karl Tomlinson (:karlt) from comment #103)
> The web platform tests seem to be fine in comment 88.
> 
> What changed between the different sets of results?
> 
> What are the new results?
> 
> I'll be able to get back to you faster if you can tell me what is wrong with
> the tests, requiring disabling the entire tests.

yes, they used to be fine earlier last week. No changes were made to this ticket that could explain such change in test result.
(try test made on November 2nd: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=674db1a795f8 all fine)

Currently they fail in a different fashion according to the platforms.
The problem is that the ini file doesn't provide fine granularity in what you can disable. You can disable a whole test or nothing.
It is otherwise expected that the test will either fail, timeout or works.
No consistent result can be expected here.
that's a try test made yesterday:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3aa78b94f800

linux-opt and linux 64 opt: same
Windows XP: extra TEST-UNEXPECTED-ERROR | /media-source/mediasource-seek-during-pending-seek.html | TypeError: info is undefined
(I've had this one occurring on my mac about 30% of the time)

For this reason, I've disabled the entire test rather than just one. no predictable outcome of a run.
Flags: needinfo?(jyavenard)
(Assignee)

Comment 105

5 years ago
This is the last try run with patches from bug 1092996, 1092979, 1093880 and 1093927 which are all related and required.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c4db27d2d075
Comment on attachment 8517218 [details] [diff] [review]
Temporarily disable mediasource-seek-beyond-duration.html webref test

(In reply to Jean-Yves Avenard [:jya] from comment #104)
> yes, they used to be fine earlier last week. No changes were made to this
> ticket that could explain such change in test result.
> (try test made on November 2nd:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=674db1a795f8 all
> fine)

This push seems to be reference as it is missing your changes,
but I bisected on Linux, identifying the patch in bug 1092996 as causing the change.

I expect the changes are all fine as the tests weren't passing anyway.

> Currently they fail in a different fashion according to the platforms.
> The problem is that the ini file doesn't provide fine granularity in what
> you can disable. You can disable a whole test or nothing.
> It is otherwise expected that the test will either fail, timeout or works.
> No consistent result can be expected here.

Individual subtests can be disabled.

It is also possible to expect different results on different platforms, but
given this is testing cross-platform code I don't have any reason to expect
different results so they may just be indicators of race conditions.

Each of these tests is too unstable (on one platform at least), and disabling
one subtest doesn't seem to resolve the problems, so yes, it seems we need to
disable all of both tests.

>Bug 1065827: Temporarily disable mediasource-seek-beyond-duration.html webref test. r=karlt

I don't know how temporary this will be.

Can you mention mediasource-seek-during-pending-seek.html also please, or just say
"disable unstable media source seek tests after ReadMetadata changes in bug 1092996"?

Mentioning one only would imply there are no others.
Attachment #8517218 - Flags: review?(karlt) → review+
(Assignee)

Updated

5 years ago
Attachment #8517218 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Blocks: 1093654
(Assignee)

Comment 108

5 years ago
Comment on attachment 8516304 [details] [diff] [review]
Fix various memory leaks in MediaDecoders

Patch moved to bug 1093654
Attachment #8516304 - Attachment is obsolete: true
(Assignee)

Comment 109

5 years ago
rebase following change of patch order
(Assignee)

Updated

5 years ago
Attachment #8517083 - Attachment is obsolete: true
(Assignee)

Comment 110

5 years ago
bug 1056441 just landed on inbound and require to rebase the lot... will need some time
Depends on: 1056441
(In reply to Jean-Yves Avenard [:jya] from comment #110)
> bug 1056441 just landed on inbound and require to rebase the lot... will
> need some time

removing c-n request for now for that
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Attachment #8516292 - Attachment is obsolete: true
(Assignee)

Comment 113

5 years ago
bump down the list
(Assignee)

Updated

5 years ago
Attachment #8516302 - Attachment is obsolete: true
(Assignee)

Comment 114

5 years ago
bump down the list
(Assignee)

Updated

5 years ago
Attachment #8516305 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8517902 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8517966 - Attachment is obsolete: true
The problem here is that with jya's changes here we're running the test in the MSE case before all segments have finished downloading, and then the test is finishing, which closes the page, which cancels pending network requests, and our "error" listener fails and reports that as a test failure.

We should just wait until the XHR request for MSE segments are finished before starting the test.

I'll upload a patch...
The XHR can fail if we finish the test before the XHR completes. Don't fail EME mochitest if this happens, just log a warning. Presumably if this is a problem, we'll timeout...

I don't think it makes sense to wait until the XHR is complete before starting the test like the MSE tests, as there are multiple segments in our MSE testcase, and so if we want to wait on events ("canplay" etc) then they could fire after the first segment is appended, and before other segments have completed download. If we waited until all segments are appended before we add event listeners etc, we could not be sure that the event listeners would fire.

Note: this failure does not appear to be the same as bug 1090523. The network requests are succeeding in that bug.
Attachment #8518501 - Flags: review?(edwin)
(Assignee)

Updated

5 years ago
Summary: MP4Reader::ReadMetaData blocks waiting for data → MediaDecoderStateMachine waits for decoded audio/video before firing loadedmetadata.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: the order to apply is: 1065827, 1092979 and 1093654
You need to log in before you can comment on or make changes to this bug.