856562 - gstreamer hangs in content/media mochitests

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Fix #1

Fixed one; more to come.

The Decode* methods don't need to guarantee that they will add anything to the video queue. It's enough to just return true if there isn't any data yet and the state machine will come back around for data if it needs it.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 731802 [details] [diff] [review]
Fix #1

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

::: content/media/gstreamer/GStreamerReader.cpp
@@ +413,5 @@
>  {
>    NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
>  
>    GstBuffer* buffer = nullptr;
>    int64_t timestamp, nextTimestamp;

Declare all those further down

Comment 3

[Alessandro Decina]

I'm going to check the patch shortly. Keep in mind though that there are two uncommitted patches that IIRC make all the tests pass (or at least did):

https://bugzilla.mozilla.org/show_bug.cgi?id=853325
https://bugzilla.mozilla.org/show_bug.cgi?id=853306

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Ah! I was under the impression that all such patches had landed already. I'll try running the tests with those patches. Still, this patch would be nice to have as it conforms a bit better to the style in the rest of Gecko.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Still hangs for me on test_streams_element_capture.html

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Oops, I introduced that hang. New patch up in a sec.

Comment 7

[Alessandro Decina]

Edwin, did you have a newer/better patch for this? I ran into this problem with the gst 1.0 port and definitely think it's time to get rid of WaitForDataBlah().

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Fix v2

Comment 9

[Alessandro Decina]

Comment on attachment 746237 [details] [diff] [review]
Fix v2

It's good with a couple of minor changes. The patch definitely fixes an issue.


>-
> bool GStreamerReader::DecodeAudioData()
> {
>   NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
> 
>-  if (!WaitForDecodedData(&mAudioSinkBufferCount)) {
>+  if (mReachedEos) {
>     mAudioQueue.Finish();
>     return false;
>   }
> 
>+  if (!mAudioSinkBufferCount) {
>+    return true;
>+  }
>+
>   GstBuffer* buffer = gst_app_sink_pull_buffer(mAudioAppSink);
>+  mAudioSinkBufferCount--;

Access to mReachedEos and mAudioSinkBufferCount should be guarded with mGstThreadsMonitor

 
>@@ -423,57 +411,58 @@ bool GStreamerReader::DecodeAudioData()
> 
> bool GStreamerReader::DecodeVideoFrame(bool &aKeyFrameSkip,
>                                          int64_t aTimeThreshold)
> {
>   NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
> 
>   GstBuffer* buffer = nullptr;
>   int64_t timestamp, nextTimestamp;

Declare these further down


>-  while (true)
>+
>+  if (mReachedEos) {
>+    mVideoQueue.Finish();
>+    return false;
>+  }
>+
>+  if (!mVideoSinkBufferCount) {
>+    return true;
>+  }
>+
>+  mDecoder->NotifyDecodedFrames(0, 1);
>+
>+  buffer = gst_app_sink_pull_buffer(mVideoAppSink);
>+  mVideoSinkBufferCount--;

Same here about mGstThreadsMonitor

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c9050008cd8

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c9050008cd8

Comment 12

[Ivaylo Dimitrov]

According to comment 1 "It's enough to just return true...". How often the state machine will come back? As I see no mon.Wait(), so it looks to me we are just wasting cycles entering and exiting DecodeVideoFrame and DecodeAudioData until something is available. This loads the CPU at maximum (verified, those functions are called tens of thousand times per second). 

Also, there was a call to NotifyBytesConsumed() that is removed by this patch. I don't know the API well enough to judge, but I guess it has been there for a reason.

I have a patch fixing the above issues, but want to hear from more knowledgeable than me on those issues before opening a new bug.

Comment 13

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

(In reply to Ivaylo Dimitrov from comment #12)
> According to comment 1 "It's enough to just return true...". How often the
> state machine will come back?

The state machine will come back straight away. It calls DecodeVideoFrame whenever the video queue is not full, so unless something else changes the logic in MediaDecoderStateMachine::DecodeLoop that causes us to call DecodeVideoFrame, we'll maybe go and call DecodeAudioData, and then call DecodeVideoFrame again pretty soon.


> As I see no mon.Wait(), so it looks to me we
> are just wasting cycles entering and exiting DecodeVideoFrame and
> DecodeAudioData until something is available. This loads the CPU at maximum
> (verified, those functions are called tens of thousand times per second). 

This sounds bad. DecodeVideoData() should be blocking until a frame is ready. I thought gst_app_sink_pull_buffer did that?

(I admit our design is pretty bad here, if the video decode takes too long the audio decode can fall behind, but it's a big job to fix our model for all backends now. You could set a time limit on how long you block instead to avoid this.)

> Also, there was a call to NotifyBytesConsumed() that is removed by this
> patch. I don't know the API well enough to judge, but I guess it has been
> there for a reason.

This is used by our buffering logic to determine whether the decoder is running faster than playback. The NotifyBytesConsumed() call should be put back!