Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[android-5.0] fix playback getting progressively slower and memory filli... #5592

Merged
merged 0 commits into from Dec 6, 2014

Conversation

Chainfire
Copy link
Contributor

...ng up until crash

The timeout in dequeueOutputBuffer has been increased to 50ms (from 5ms), to ensure a buffer will be available. While it appears this is not an issue on 4.4 and older, on Android 5.0 dequeueOutputBuffer with a 5ms timeout will usually fail, releaseOutputBuffer will not be called, and this will lead to most dequeueInputBuffer calls failing as well (far fewer than wanted fps calls will succeed). This in turn leads to the m_demux queue quickly filling up with the raw frame data, consuming all system memory, until a crash results.

@Chainfire
Copy link
Contributor Author

Tested on:

  • Nexus 7: Android 5.0
  • ADT-1: Android 5.0
  • Nexus 5: Android 4.4

Successfully fixes heavy stuttering and crashing on the Android 5.0 devices, no difference noted on the 4.4 device.

This only pertains to MediaCodec based decoding, should have noted that somewhere in the commit, I guess.

@wsnipex
Copy link
Member

wsnipex commented Oct 28, 2014

thanks!
@koying @davilla ping

@koying
Copy link
Contributor

koying commented Oct 28, 2014

IIRC @davilla already adjusted this value for AFTV.
50ms (i.e. > 1 frame) seems much, though. Did you try other values?

@Chainfire
Copy link
Contributor Author

Yes, tried a few different values, 20ms for example didn't work well for me. Could be that >= 1 frame is actually the magic.

There's some other things I may want to look into fixing for my ADT-1, so if @davilla is also working on that, please point me at the changes he made so I'm not wasting my time...

@koying
Copy link
Contributor

koying commented Oct 28, 2014

@davilla is no more in the team itself, we are just asking for his enlightened advice, so if you have other stuff in mind, please go on.

It makes sense to wait for a picture the necessary time here, although I'd be much more comfortable not waiting more than a frame duration.
A further precaution would be to not internally buffer more than the number of mediacodec input buffers. m_demux.size() > m_input.size() doesn't seem useful.

@wsnipex
Copy link
Member

wsnipex commented Dec 4, 2014

@koying ping. This is still an issue.

@koying
Copy link
Contributor

koying commented Dec 4, 2014

+1

@wsnipex wsnipex added Helix Type: Fix non-breaking change which fixes an issue labels Dec 4, 2014
@MartijnKaijser
Copy link
Member

well the PR needs some cleanup to get rid of the merge/master commits :)

@davilla
Copy link
Contributor

davilla commented Dec 4, 2014

This timeout should be tied to the version of android unless you want to break older android versions. The version test is easy and quick, setup the timeout when mediacodec is created.

@koying
Copy link
Contributor

koying commented Dec 4, 2014

PR welcome, you know best ;)

@davilla
Copy link
Contributor

davilla commented Dec 4, 2014

Thinking on this PR, there's something else going on and this PR is a band-aid on it. Most likely behavior changes that Google made to MediaCodec in 5.0 to support their async callback for processing buffers. My bet is they broke something or something MediaCodec does is not quite right now.

As for PRs, sorry, I don't do PRs for Kodi but you are certainly welcome to cherry-pick from Pivos fork when we push out.

@koying
Copy link
Contributor

koying commented Dec 4, 2014

OK. Let's go with the band-aid for Helix.

It's apparent that a too small timeout causes issues.
Do you see issues with having a too great timeout?

@wsnipex
Copy link
Member

wsnipex commented Dec 4, 2014

@koying
Copy link
Contributor

koying commented Dec 4, 2014

Probably not. Asus Flo is Nexus 7.

@wsnipex
Copy link
Member

wsnipex commented Dec 5, 2014

@Chainfire please rebase/remove the merge commits

@Chainfire
Copy link
Contributor Author

Clean PR here: #5891

@MartijnKaijser MartijnKaijser merged commit ecd5425 into xbmc:master Dec 6, 2014
@Chainfire
Copy link
Contributor Author

That PR got deleted, sorry, new one here: #5898

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants