gstreamer - high CPU usage after patch for Bug 856562 landed

RESOLVED FIXED in mozilla24

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ivaylo Dimitrov, Assigned: Ivaylo Dimitrov)

Tracking

unspecified
mozilla24
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 758849 [details] [diff] [review]
Wait in DecodeAudioData/DecodeVideoFrame until data is avalable

Follow-up to https://bugzilla.mozilla.org/show_bug.cgi?id=856562#c13
(Assignee)

Comment 1

5 years ago
Comment on attachment 758849 [details] [diff] [review]
Wait in DecodeAudioData/DecodeVideoFrame until data is avalable

The patch adds NotifyBytesConsumed() call back
Attachment #758849 - Flags: review?(cpearce)
Comment on attachment 758849 [details] [diff] [review]
Wait in DecodeAudioData/DecodeVideoFrame until data is avalable

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

I don't know enough about GStreamer or our GStreamerReader to properly review this patch, Alessandro should review it.

The idea is good though.

::: content/media/gstreamer/GStreamerReader.cpp
@@ +441,3 @@
>      if (!mAudioSinkBufferCount) {
> +      if(!mVideoSinkBufferCount) {
> +        mon.Wait();

What notifies this monitor if the reader is shutdown while we're waiting here? When the reader is shutdown, the MediaResource::Read etc functions will start to fail. Does GStreamerReader::Eos get called when this happens? Alessandro will know this.

You also might want to make this a mon.Wait(PR_MillisecondsToInterval(40)) so that if the video decode is taking too long we bail out and return to the DecodeLoop to try audio decoding.

@@ +499,5 @@
> +        if (!mVideoSinkBufferCount) {
> +          return true;
> +        }
> +      }
> +      else {

} else {
Attachment #758849 - Flags: review?(cpearce) → review?(alessandro.d)
(Assignee)

Comment 3

5 years ago
> You also might want to make this a mon.Wait(PR_MillisecondsToInterval(40))
> so that if the video decode is taking too long we bail out and return to the
> DecodeLoop to try audio decoding.
> 
> @@ +499,5 @@
> > +        if (!mVideoSinkBufferCount) {
> > +          return true;
> > +        }
> > +      }
> > +      else {
> 
> } else {

mon.NotifyAll() is called in both NewVideoBuffer() and NewAudioBuffer(), we block on mon.Wait() only if there is neither video nor audio data, so whichever data comes first will make mon.Wait() to exit. i.e. if we are blocked in DecodeVideoFrame and new audio buffer is available, then NewAudioBuffer will call mon.NotifyAll(), which will result in DecodeVideoFrame returning to state machine as mVideoSinkBufferCount is 0, the same logic applies if we are blocked in DecodeAudioData and new video buffer is ready. I don't like the idea of putting timeouts there, the logic behind this patch is (IMO) robust enough to deal with slow video decoding.
Cool, that sounds like we can handle slow video decoding. Thanks for explaining that.

What about the case where the MediaResource is closed and reads/seeks start failing?
(Assignee)

Comment 5

5 years ago
> What about the case where the MediaResource is closed and reads/seeks start
> failing?

I'd prefer to wait for Alessandro's comment on that one as I don't see any difference with the state before the patch for Bug 856562 landed, so I guess this is handled somehow. If it is not, the obvious workaround (for me) would be to replace mon.Wait() with mon.Wait(PR_MillisecondsToInterval(N)), no matter how much I dislike the timeouts approach.

Comment 6

5 years ago
(In reply to Ivaylo Dimitrov from comment #5)
> > What about the case where the MediaResource is closed and reads/seeks start
> > failing?
> 
> I'd prefer to wait for Alessandro's comment on that one as I don't see any
> difference with the state before the patch for Bug 856562 landed, so I guess
> this is handled somehow. If it is not, the obvious workaround (for me) would
> be to replace mon.Wait() with mon.Wait(PR_MillisecondsToInterval(N)), no
> matter how much I dislike the timeouts approach.

Yes when Read() fails we signal EOS calling gst_app_src_end_of_stream, see ::ReadAndPushData.

Comment 7

5 years ago
Comment on attachment 758849 [details] [diff] [review]
Wait in DecodeAudioData/DecodeVideoFrame until data is avalable

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

r+, but please add a comment in the source explaining why we wait only when neither video nor audio are available
Attachment #758849 - Flags: review?(alessandro.d) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 760135 [details] [diff] [review]
Wait in DecodeAudioData/DecodeVideoFrame until data is avalable v2

Added comments
Attachment #758849 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #760135 - Flags: checkin?
Status: UNCONFIRMED → NEW
Ever confirmed: true
https://hg.mozilla.org/mozilla-central/rev/750cf3ffac80
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Attachment #760135 - Flags: checkin? → checkin+
You need to log in before you can comment on or make changes to this bug.