go-mp3 icon indicating copy to clipboard operation
go-mp3 copied to clipboard

Opened Context consumes 100% CPU

Open dimonomid opened this issue 5 years ago • 13 comments

Hi @hajimehoshi ,

First of all, thanks a lot for your awesome work on go-mp3.

I'm on linux, and while trying it out, I noticed that after the oto.Context has been created, CPU is always at 100% load. E.g. I was following your example https://github.com/hajimehoshi/go-mp3/blob/master/example/main.go , and wrote a function which just plays some mp3 file once:

func playBeep() error {
	f, err := assets.Open("beep.mp3")
	if err != nil {
		return err
	}
	defer f.Close()

	d, err := mp3.NewDecoder(f)
	if err != nil {
		return err
	}
	defer d.Close()

	p, err := oto.NewPlayer(d.SampleRate(), 2, 2, 8192)
	if err != nil {
		return err
	}
	defer p.Close()

	if _, err := io.Copy(p, d); err != nil {
		return err
	}
	return nil
}

And I noticed that after this function was called the first time, my app starts consuming 100% CPU. After some investigation, I found out that oto.NewPlayer() creates a context which is then never closed, and this context is what actually consumes CPU.

However, the code suggests that the context should only be created once: https://github.com/hajimehoshi/oto/blob/master/context.go#L59-L61

	if theContext != nil {
		panic("oto: NewContext can be called only once")
	}

And I noticed that theContext isn't really used anywhere, it only prevents contexts from being created more than once. So I tried to remove that limitation (thus allowing context to be create multiple times), so my playBeep function looks as follows:

func playBeep() error {
	f, err := assets.Open("beep.mp3")
	if err != nil {
		return err
	}
	defer f.Close()

	d, err := mp3.NewDecoder(f)
	if err != nil {
		return err
	}
	defer d.Close()

	c, err := oto.NewContext(d.SampleRate(), 2, 2, 8192)
	if err != nil {
		return err
	}
	defer c.Close()

	p := c.NewPlayer()
	defer p.Close()

	if _, err := io.Copy(p, d); err != nil {
		return err
	}
	return nil
}

And it seems to work fine: plays the sound ever time it's called, and CPU is okay. So, what was the reason for that theContext?

dimonomid avatar Feb 15 '19 13:02 dimonomid

Hi @dimonomid, thank you for reporting!

That sounds a bug in Oto: CPU must not be busy when nothing is played. Let me check. Thank you!

hajimehoshi avatar Feb 15 '19 14:02 hajimehoshi

Ah ok, oto.NewPlayer creates a context and a player at the same time, while (*oto.Context).NewPlayer creates a player from a context and this can be called multiple times.

In your case, I think you need only one Context in one process, so would this work?


var theContext *oto.Context

// This is called only once
func initializeContext() error {
        var err error
        theContext, err = oto.NewContext(d.SampleRate(), 2, 2, 8192)
	if err != nil {
		return err
	}
        return nil
}

func playBeep() error {
	f, err := assets.Open("beep.mp3")
	if err != nil {
		return err
	}
	defer f.Close()

	d, err := mp3.NewDecoder(f)
	if err != nil {
		return err
	}
	defer d.Close()

	p := theContext.NewPlayer()
	defer p.Close()

	if _, err := io.Copy(p, d); err != nil {
		return err
	}
	return nil
}

Then you don't have to close (and recreate) the context.

hajimehoshi avatar Feb 15 '19 14:02 hajimehoshi

Thanks for looking into that!

Not sure what do you mean though; the problem persists: as long as Context is created and not closed, CPU is at 100%. And since in that last example we never close theContext, CPU is always at 100%.

dimonomid avatar Feb 15 '19 14:02 dimonomid

That's odd. I opened a context and wait for 10 seconds, but CPU usage is not changed. Maybe playing something is needed?

hajimehoshi avatar Feb 15 '19 15:02 hajimehoshi

So it doesn't reproduce on your machine?

I've put together a small repro: https://github.com/dimonomid/mp3test (based on the snippet you posted above)

Just go build && ./mp3test there, and it plays a short sound for me, then sleeps for a minute, and during that sleep CPU is loaded 100%. If I comment the playing part, it's still at 100%. Closing the context does help.

Can you reproduce?

dimonomid avatar Feb 15 '19 23:02 dimonomid

Thanks. I could reproduce this. Let me investigate this. Thank you!

hajimehoshi avatar Feb 16 '19 01:02 hajimehoshi

I found the bug in Oto and fixed this: https://github.com/hajimehoshi/oto/commit/3081a399fb4115016dd6bb8252209f729c819541

Could you test this? Thank you!

hajimehoshi avatar Feb 16 '19 01:02 hajimehoshi

It's much better now, but still not ideal: it keeps steadily consuming 5% CPU. If I close the context after playing, it'll be 0% (which is what a sleeping program should do).

dimonomid avatar Feb 16 '19 09:02 dimonomid

it keeps steadily consuming 5% CPU.

My guess is that Oto tries to play 'no sound' when there is no player in the current implementation. Well, suspending the underlying driver during absence of players might work. Let me think.

hajimehoshi avatar Feb 16 '19 10:02 hajimehoshi

Same problem here. I'm not an expert, but if I find a solution I'm going to share it

ackFacu avatar Aug 16 '19 02:08 ackFacu

I made something similar to this and I had no problems. Just a 0,24% cpu usage and 4mb of ram, so idk if this is still a valid issue

func PlayTimeLimit() {
	file, _ := os.Open("notify.mp3")
	defer file.Close()
	decoder, _ := mp3.NewDecoder(file)

	once.Do(func() {
		context, _ = oto.NewContext(decoder.SampleRate(), 2, 2, 8192)
	})

	p := context.NewPlayer()
	defer p.Close()

	if _, err := io.Copy(p, decoder); err != nil {
		return
	}
	return
}

ackFacu avatar Aug 21 '19 02:08 ackFacu

This was already solved, not perfectly though. Theoretically, CPU usage can be 0% when there is no player.

hajimehoshi avatar Aug 21 '19 03:08 hajimehoshi

I made something similar to this and I had no problems. Just a 0,24% cpu usage and 4mb of ram, so idk if this is still a valid issue

func PlayTimeLimit() {
	file, _ := os.Open("notify.mp3")
	defer file.Close()
	decoder, _ := mp3.NewDecoder(file)

	once.Do(func() {
		context, _ = oto.NewContext(decoder.SampleRate(), 2, 2, 8192)
	})

	p := context.NewPlayer()
	defer p.Close()

	if _, err := io.Copy(p, decoder); err != nil {
		return
	}
	return
}

Thank you very much,This works.

donething avatar Jan 07 '20 04:01 donething