880029 - gstreamer - high CPU usage after patch for Bug 856562 landed

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ivaylo Dimitrov]

Attachment: Wait in DecodeAudioData/DecodeVideoFrame until data is avalable

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

Comment 1

[Ivaylo Dimitrov]

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

The patch adds NotifyBytesConsumed() call back

Comment 2

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

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 {

Comment 3

[Ivaylo Dimitrov]

> 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.

Comment 4

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

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?

Comment 5

[Ivaylo Dimitrov]

> 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

[Alessandro Decina]

(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

[Alessandro Decina]

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

Comment 8

[Ivaylo Dimitrov]

Attachment: Wait in DecodeAudioData/DecodeVideoFrame until data is avalable v2

Added comments

Comment 9

[Oleg Romashin (:romaxa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/750cf3ffac80

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/750cf3ffac80