libGDX game crashes on ART, Google's successor for dalvik. #1939

Merged
merged 9 commits into from Apr 10, 2015

Projects

None yet

6 participants

@schleinzer
Contributor

Reason is the allocation of a ByteBuffer of size "0" in "IndexBufferObject". This breaks all libGDX Meshes which don't make use of indexes and set the "maxIndexes" to 0 (which in turn breaks quite a lot of libGDX apps on ART I assume.)

While allocating a 0-sized ByteBuffer is perfectly fine according to the javadoc, ART has a bug that makes programs crash if they try to do so. Only sizes >0 do work.

This is a bug in ART admitted and even fixed by Google with https://android-review.googlesource.com/#/c/73175/ .

Unfortunately, that fix is (although committed half a year ago...) not even part of current Android (4.4.3 as of this writing), and it's still unknown (to me) in which release it will finally be available.

So I kindly ask you to accept this pull request to work around that problem for the time being.

Thank you!

Thorsten Schleinzer libGDX game crashes on ART, Google's successor for dalvik.
Reason is the allocation of a ByteBuffer of size "0" in "IndexBufferObject". This breaks all libGDX Meshes which don't make use of indexes and set the "maxIndexes" to 0 (which in turn breaks quite a lot of libGDX apps on ART I assume.)

While allocating a 0-sized ByteBuffer is perfectly fine according to the javadoc, ART has a bug that makes programs crash if they try to do so. Only sizes >0 do work.

This is a bug in ART admitted and even fixed by Google with https://android-review.googlesource.com/#/c/73175/ .

Unfortunately, that fix is (although committed half a year ago...) not even part of current Android (4.4.3 as of this writing), and it's still unknown (to me) in which release it will finally be available.

So I kindly ask you to accept this pull request to work around that problem for the time being.

Thank you!
23ac1ed
@johnnyapol
Member

I believe several pull requests have been rejected in the past about this. I have no idea why they [Google] didn't include the fix in 4.4.3.

I'll let @badlogic and @MobiDevelop handle this.

EDIT:
It's a little alarming that the fix wasn't included in the build for 4.4.3 even when the fix was committed in December

EDIT 2: Just checked the master branch of platform/art, the fix is still present there. I'm assuming the 4.4.3 release was full of cherry-picked commits Google wanted specifically, looking at the leaked changelog.

@schleinzer
Contributor

Yes, I don't have a clue with this commit lies dormant now for over six months...

Fact is, it is a plot stopper for me, making my game (and I guess many more libGDX apps) crash on start on ART, forcing me to link to my own fork of libGDX, something I'd rather not do...

So I'd be happy to see my pull request accepted as soon as possible (or any other way fixing the problem, that is).

Thank you!

@MobiDevelop
Member

The reason a workaround has not been implemented lies mostly in the fact that ART is an experimental not-fully-baked runtime that few people know exists, let alone use. If we added workarounds for every broken thing on every device/runtime/platform/whatever we'd have quite the messy codebase.

I personally would rather not spend time accommodating ART until Google decides to prioritize that runtime (the lack of an updated ART runtime in 4.4.3 suggests it is not currently a priority for them).

All things considered, if the only thing preventing libgdx games from running on ART is this one thing, I don't see much harm in integrating the workaround. Will let @badlogic decide on that.

@schleinzer
Contributor

I totally agree with you regarding polluting the codebase with workarounds for all kinds of problems - would never want to do that myself.

On the other hand, from what I read in the commits of ART code, Google in fact seems to start to greatly prioritize ART and am convinced it will be the standard for new devices quite soon.

Also I don't see the lack of an updated ART runtime in 4.4.3 - it's just that this commit (and some more I assume) were not integrated while cherry picking things...

Your last statement takes the words right out of my mouth. As far my research went (have been looking for references to BufferUtils and creating 0-sized buffers), my workaround indeed should be enough to fix things for ART - at least my game runs flawlessly after applying it.

So, still, I'd really love to see a fix :-) Thank you!

@badlogic
Member
badlogic commented Jun 8, 2014

It's indeed weird that they didn't merge that fix. Your fix will work for the case where there are no indices in a mesh. I suspect a lot more cases to crop up like that (zero element buffers). That would mean further littering our code base with work arounds. I'm afraid we won't support ART until Google says it's the defacto standard.

@badlogic badlogic closed this Jun 8, 2014
@schleinzer
Contributor

Hi Mario

sorry to read that.

You are right about the cases were 0 element buffers might crop up, difference is, when users do that in their code, they can fix it. While there are some places within libGDX (ShapeRenderer, for example) users can't fix w/o forking the code... quite a burden...

As far as my research went, my workaround (won't call it a fix...) should deal with all the current cases. So I am really with "MobilDevelop"'s last statement here...

But it's your (great!) code and if you won't fix it, I will - heavy-heartedly - keep on using my fork instead... as I don't agree and in fact consider it crucial that my games also works with ART even now...

(Oh, and to add to the "gradle discussion" - it's a nice, but heavy tool, especially making it real hard in my case to use my fork instead of the "real thing"...)

Thorsten Sch... added some commits Jun 19, 2014
Thorsten Schleinzer Merge remote-tracking branch 'remotes/upstream/master' d83364d
Thorsten Schleinzer Merge tag 'gdx-parent-1.2.0'
[maven-release-plugin]  copy for tag gdx-parent-1.2.0
d8acea8
@Igor1201

I know it has been closed with other ART-related issues but yesterday Google's announced ART will be default on Android L.
When they release the source we can check it for sure but now it's interesting to consider fixing it from here.

@johnnyapol
Member

@Igor1201

I'm testing the ART behavior as we speak with LibGDX on the dev preview.

@Igor1201

Nice! Please keep us updated!

@johnnyapol
Member

@Igor1201

This issue is no longer present on the developer preview, the commit that fixed it must have been included. I tested my game along with a ByteBuffer allocation test I wrote and everything is working fine. :)

@Igor1201

@Unkn0wn0ne
Thank you very much, looking forward to getting my hands on this dev preview!

@johnnyapol
Member

@Igor1201

No problem! It was my pleasure.

@schleinzer
Contributor

Hi,

as this issue still makes crash most libGDX games on Android < 5 with ART enabled (not actually a rarity nowadays), could you - pretty please - reconsider accepting the pull request?

I have a game on the Play Store for more than 6 months now with that PR applied (using my own fork of libGDX just for this...) without any problems, so it may be considered safe.

I would really love to be able to get rid of my own fork just for this.

Thank you very much!

@NathanSweet
Member

It's been fixed in ART, so this is only an issue for old devices. I believe @badlogic's resistance comes from the fact that, while your particular app works with only this workaround, there are probably still other areas that try to allocate a zero length buffer. That means it's likely that other apps still couldn't use ART with libgdx on <5 devices. One hack might be ok, but adding any more of these kinds of hacks for ART bugs isn't really acceptable.

Possibly you can find a way to move the workaround into application code? Or just avoid buggy ART on <5 devices?

@schleinzer
Contributor

Thanks, Nathan, for your elaborate answer.

I totally get your point(s) and feel with you (nobody likes hacks of that kind, me neither...).

I did try to fix this with application code but it was (AFAIU) not possible.

As for the other occurrences of allocations of zero length buffers, you might be right - you are far more into the code base than I am. But for the allocations in question, I did a thorough search and "fixed" all possible cases. Again, my game makes use of quite a lot of libGDX's features and runs flawlessly on Android 4.4.4 with ART enabled.

Furthermore, the "hack" is very, very simple with an absolutely neglectable performance impact (if any).

As for "avoiding buggy ART on devices <5" - that, unfortunately, is not us developers to choose but users who switch their runtimes; so nothing I can actually do about it, but let my game crash for those users...

It is your (awesome!) code and it's perfectly fine if you decide not to accept my PR (as you did before), but being it such a simple fix and you saying "one hack might be ok", why not accept this one and hope it fixes most (all?) other applications as well :-} ?

Thanks again, both for your awesome support and for libGDX!

@badlogic
Member

Do you have crash statistics from your users? Would be interesting to know
how many people are affected by turning on the experimental ART VM in 4.x.
On Mar 23, 2015 4:31 PM, "Thorsten Schleinzer" notifications@github.com
wrote:

Thanks, Nathan, for your elaborate answer.

I totally get your point(s) and feel with you (nobody likes hacks of that
kind, me neither...).

I did try to fix this with application code but it was (AFAIU) not
possible.

As for the other occurrences of allocations of zero length buffers, you
might be right - you are far more into the code base than I am. But for the
allocations in question, I did a thorough search and "fixed" all possible
cases. Again, my game makes use of quite a lot of libGDX's features and
runs flawlessly on Android 4.4.4 with ART enabled.

Furthermore, the "hack" is very, very simple with an absolutely
neglectable performance impact (if any).

As for "avoiding buggy ART on devices <5" - that, unfortunately, is not us
developers to choose but users who switch their runtimes; so nothing I can
actually do about it, but let my game crash for those users...

It is your (awesome!) code and it's perfectly fine if you decide not to
accept my PR (as you did before), but being it such a simple fix and you
saying "one hack might be ok", why not accept this one and hope it fixes
most (all?) other applications as well :-} ?

Thanks again, both for your awesome support and for libGDX!


Reply to this email directly or view it on GitHub
#1939 (comment).

@schleinzer
Contributor

Sorry, I don't have such statistics as I fixed the problem before releasing the game.

But I know that, for example, the NVIDIA SHIELD PORTABLE is often configured to use ART (performance matters for a gaming console) and was informed that a lot of early "Android TV" devices use <5 with ART enabled. This information is about half a year old, so may not be that accurate anymore. But still, there are a lot of devices <5 and I dare to assume quite a few are configured to use ART.

Thanks for taking the time, discussing that matter!

@badlogic badlogic reopened this Mar 24, 2015
@badlogic badlogic commented on the diff Mar 24, 2015
...src/com/badlogic/gdx/graphics/glutils/IndexArray.java
@@ -28,10 +28,19 @@
ShortBuffer buffer;
ByteBuffer byteBuffer;
+ // used to work around bug: https://android-review.googlesource.com/#/c/73175/
+ private final boolean empty;
@badlogic
badlogic Mar 24, 2015 Member

we don't need this flag

@schleinzer
schleinzer Mar 25, 2015 Contributor

Mhmmm... actually, "maxIndices" is not a field either, but would have to be to be checked in the getters. So it's either "maxIndices" or "empty" which would need to be added as a field. Actually, I tend to find my original code more self-explanatory :-] Please tell me how to proceed.

@badlogic badlogic commented on the diff Mar 24, 2015
...src/com/badlogic/gdx/graphics/glutils/IndexArray.java
/** Creates a new IndexArray to be used with vertex arrays.
*
* @param maxIndices the maximum number of indices this buffer can hold */
public IndexArray (int maxIndices) {
+
+ empty = maxIndices == 0;
@badlogic
badlogic Mar 24, 2015 Member

Instead of using the empty flag, simply set the limit to 0 after the buffer has been created. Please test this.

@badlogic badlogic commented on the diff Mar 24, 2015
...src/com/badlogic/gdx/graphics/glutils/IndexArray.java
}
/** @return the maximum number of indices this IndexArray can store. */
public int getNumMaxIndices () {
- return buffer.capacity();
+ return empty ? 0 : buffer.capacity();
@badlogic
badlogic Mar 24, 2015 Member

return maxindices == 0? 0: buffer.capacity();

@schleinzer
schleinzer Mar 24, 2015 Contributor

Would the previous simple "return buffer.limit();" not suffice if we, as you suggest, set the buffer's limit to "0" after "fake creating" it with a size>0 - EDIT: I actually only meant the first case in "getNumIndices". (Figured I can't "move" my comment...)

@badlogic
badlogic Mar 24, 2015 Member

No, because the limit may be != capacity.
On Mar 24, 2015 6:16 PM, "Thorsten Schleinzer" notifications@github.com
wrote:

In gdx/src/com/badlogic/gdx/graphics/glutils/IndexArray.java
#1939 (comment):

}

/** @return the maximum number of indices this IndexArray can store. */
public int getNumMaxIndices () {
  •   return buffer.capacity();
    
  •   return empty ? 0 : buffer.capacity();
    

Would the previous simple "return buffer.limit();" not suffice if we, as
you suggest, set the buffer's limit to "0" after "fake creating" it with a
size>0


Reply to this email directly or view it on GitHub
https://github.com/libgdx/libgdx/pull/1939/files#r27050726.

@badlogic badlogic and 1 other commented on an outdated diff Mar 24, 2015
.../badlogic/gdx/graphics/glutils/IndexBufferObject.java
buffer = byteBuffer.asShortBuffer();
buffer.flip();
byteBuffer.flip();
bufferHandle = createBufferObject();
- usage = GL20.GL_STATIC_DRAW;
+ usage = isStatic ? GL20.GL_STATIC_DRAW : GL20.GL_DYNAMIC_DRAW;
@badlogic
badlogic Mar 24, 2015 Member

This switches our previous default of static draw to dynamic draw. Please don't do this.

@schleinzer
schleinzer Mar 24, 2015 Contributor

Could explain why this is? The "isStatic" is always set and was ignored before my change. Or, putting it another way, how does one than ever switch to dynamic draw? Thank you! (Really don't want to bother you, just want to make sure to understand mistakes I make...)

@badlogic
badlogic Mar 24, 2015 Member

Ah, ignore me, long day. Your fix is fine.
On Mar 24, 2015 6:20 PM, "Thorsten Schleinzer" notifications@github.com
wrote:

In gdx/src/com/badlogic/gdx/graphics/glutils/IndexBufferObject.java
#1939 (comment):

    buffer = byteBuffer.asShortBuffer();
    buffer.flip();
    byteBuffer.flip();
    bufferHandle = createBufferObject();
  •   usage = GL20.GL_STATIC_DRAW;
    
  •   usage = isStatic ? GL20.GL_STATIC_DRAW : GL20.GL_DYNAMIC_DRAW;
    

Could explain why this is? The "isStatic" is always set and was ignored
before my change. Or, putting it another way, how does one than ever switch
to dynamic draw? Thank you! (Really don't want to bother you, just want to
make sure to understand mistakes I make...)


Reply to this email directly or view it on GitHub
https://github.com/libgdx/libgdx/pull/1939/files#r27051160.

@badlogic badlogic commented on an outdated diff Mar 24, 2015
.../badlogic/gdx/graphics/glutils/IndexBufferObject.java
- buffer = byteBuffer.asShortBuffer();
- buffer.flip();
- byteBuffer.flip();
- bufferHandle = createBufferObject();
- usage = isStatic ? GL20.GL_STATIC_DRAW : GL20.GL_DYNAMIC_DRAW;
- }
+ empty = maxIndices == 0;
@badlogic
badlogic Mar 24, 2015 Member

Same as above, remove empty flag, set limit to 0 after buffer creation.

@badlogic badlogic commented on the diff Mar 24, 2015
.../badlogic/gdx/graphics/glutils/IndexBufferObject.java
@@ -92,12 +94,12 @@ private int createBufferObject () {
/** @return the number of indices currently stored in this buffer */
public int getNumIndices () {
- return buffer.limit();
+ return empty ? 0 : buffer.limit();
@badlogic
badlogic Mar 24, 2015 Member

same as above, use maxindices

@badlogic badlogic commented on the diff Mar 24, 2015
.../badlogic/gdx/graphics/glutils/IndexBufferObject.java
}
/** @return the maximum number of indices this IndexBufferObject can store. */
public int getNumMaxIndices () {
- return buffer.capacity();
+ return empty ? 0 : buffer.capacity();
@badlogic
badlogic Mar 24, 2015 Member

same as above, use maxindices

@badlogic badlogic commented on an outdated diff Mar 24, 2015
.../badlogic/gdx/graphics/glutils/IndexBufferObject.java
@@ -139,7 +141,9 @@ public ShortBuffer getBuffer () {
/** Binds this IndexBufferObject for rendering with glDrawElements. */
public void bind () {
- if (bufferHandle == 0) throw new GdxRuntimeException("No buffer allocated!");
+ if (bufferHandle == 0) {
+ throw new GdxRuntimeException("No buffer allocated!");
+ }
@badlogic
badlogic Mar 24, 2015 Member

Please don't add curlies where there where no curlies before.

@badlogic
Member

I commented on your PR. Please fix it up and make sure it merges cleanly with master. I'll merge this then.

@schleinzer schleinzer commented on the diff Mar 24, 2015
...src/com/badlogic/gdx/graphics/glutils/IndexArray.java
@@ -40,12 +49,12 @@ public IndexArray (int maxIndices) {
/** @return the number of indices currently stored in this buffer */
public int getNumIndices () {
- return buffer.limit();
+ return empty ? 0 : buffer.limit();
}
@schleinzer
schleinzer Mar 24, 2015 Contributor

This is the place I actually referred to :-)

@badlogic
Member

Oki, we had some talk off list. I'll merge this if it cleanly merges with master.

@schleinzer
Contributor

Hi,

Mario agreed to merge this PR - are there any news on this?

Thank you!

@NathanSweet
Member

It can't merge automatically. The PR needs to be updated to the latest libgdx.

Thorsten Sch... and others added some commits Apr 10, 2015
Thorsten Schleinzer - Merged with current master
- Reformatted code according to style guide.
f7c7ba2
Thorsten Schleinzer Merge remote-tracking branch 'remotes/upstream/master'
Conflicts:
	gdx/src/com/badlogic/gdx/graphics/glutils/IndexBufferObject.java
89038f5
Thorsten Schleinzer Merge branch 'master' into make_it_run_on_atr 5b47bbe
Thorsten Schleinzer Merged correctly ca29fdf
@schleinzer schleinzer Update org.eclipse.jdt.ui.prefs 341c6b1
@schleinzer
Contributor

Just redid the merge properly and also applied the correct "libgdx" formatter to honor your style guide.

Should merge cleanly now.

@NathanSweet
Member

Now we wait for @badlogic 😄

@badlogic
Member

Merge away, revert if thinksmbreak.
On Apr 10, 2015 5:23 PM, "Nathan Sweet" notifications@github.com wrote:

Now we wait for @badlogic https://github.com/badlogic [image: 😄]


Reply to this email directly or view it on GitHub
#1939 (comment).

@NathanSweet NathanSweet merged commit dca16d3 into libgdx:master Apr 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment