Closed Bug 856562 Opened 7 years ago Closed 7 years ago

gstreamer hangs in content/media mochitests

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: eflores, Assigned: eflores)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch Fix #1 (obsolete) — Splinter Review
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.
Assignee: nobody → edwin
Status: NEW → ASSIGNED
Attachment #731802 - Flags: review?(alessandro.d)
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
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
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.
Still hangs for me on test_streams_element_capture.html
Oops, I introduced that hang. New patch up in a sec.
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().
Attached patch Fix v2Splinter Review
Attachment #731802 - Attachment is obsolete: true
Attachment #731802 - Flags: review?(alessandro.d)
Attachment #746237 - Flags: review?(alessandro.d)
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
Attachment #746237 - Flags: review?(alessandro.d) → review+
https://hg.mozilla.org/mozilla-central/rev/6c9050008cd8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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.
(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!
You need to log in before you can comment on or make changes to this bug.