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

Texture atlas GL error 1285 OUT_OF_MEMORY #404

Closed
devemux86 opened this issue Sep 18, 2017 · 38 comments
Closed

Texture atlas GL error 1285 OUT_OF_MEMORY #404

devemux86 opened this issue Sep 18, 2017 · 38 comments
Labels

Comments

@devemux86
Copy link
Collaborator

Testing texture atlas on Android via AtlasThemeActivity at some devices we have out of memory errors, making the map unusable:

org.oscim.android.test E/org.oscim.renderer.GLUtils: finish: glError 1285
@devemux86 devemux86 added the bug label Sep 18, 2017
@devemux86
Copy link
Collaborator Author

cc @Longri

@Longri
Copy link

Longri commented Sep 18, 2017

Which devices?
maybe texture are to big and wie must check max TextureSize

@devemux86
Copy link
Collaborator Author

Tested on some available devices Samsung I9100 Galaxy S II and Samsung Galaxy Tab 4 7.0 LTE, both with Mali-400 GPU.

@Longri
Copy link

Longri commented Sep 18, 2017

do you have such divices?

Cann you try to set MAX_ATLAS_SIZE to 1024?

1

@devemux86
Copy link
Collaborator Author

Already check that, the OOM seem to be much less even with MAX_ATLAS_SIZE = 128!

BTW forgot to mention that the issue appears mostly in dense areas around zoom 15 where symbols start appearing.

@Longri
Copy link

Longri commented Sep 18, 2017

I'm trying to reactivate my old S2, which I found deep in my closet.
Then I'll try to find the mistake.

@devemux86
Copy link
Collaborator Author

@Longri have you tested again texture atlas?

@Longri
Copy link

Longri commented May 12, 2018

I'm sorry, I totally forgot.

I wrote a library that queries the maximum size of an OpenGLxture and then, based on this size, creates an atlas.

The positions in the atlas are calculated using Native.

I wanted to test this library thoroughly before creating a PR.

And then I just forgot.

The library I wrote is based on LibGdx. I will rewrite them now and remove all LibGdx dependencies to create a PR.

The advantage of this library is that the maximum size is used. And the pack algorithm is very effective here and comes from : https://github.com/TeamHypersomnia/rectpack2D

@devemux86
Copy link
Collaborator Author

Indeed that seems the safest approach!

@Longri
Copy link

Longri commented May 16, 2018

Who compiled the native sources last time?

I tried to repair the JNI module and made the necessary changes in the JinGen.
But I can't get all platforms compiled.
I just:

  • Win64
  • Android => x86
  • Android => x86_64
  • Android => armeabi
  • Android => armeabi-v7a
  • Android => arm64-v8a
  • Linux64
  • Mac64
  • Mac32
  • Ios

So it's missing:

  • Win32
  • Linux32
  • Android=>mips (maybe I need to install the mips-NDK)
  • Android=>mips64

For Mips I will try to install the Mips-NDK.

But are Linux32 and Win32 really necessary? I would also leave out Mac32.

@devemux86
Copy link
Collaborator Author

devemux86 commented May 16, 2018

Who compiled the native sources last time?

I did all native builds in #14 when forked VTM.
On Linux for all platforms and on Mac.

(If there is need I'll do that again)

But are Linux32 and Win32 really necessary? I would also leave out Mac32.

Since there are users still using 32bit OS, we need to provide support for all.

But first must understand:

I tried to repair the JNI module and made the necessary changes in the JinGen.

Why and what needs to be changed?

@Longri
Copy link

Longri commented May 16, 2018

Why and what needs to be changed?

The'build-ios.xml.template' contains an incorrect path to the iOS SDK!

Since there are users still using 32bit OS, we need to provide support for all.

I don't think anyone else uses 32Bit, but I will do my best to compile the missing platforms as well.

@devemux86
Copy link
Collaborator Author

devemux86 commented May 16, 2018

I didn't build iOS - you maintain it.
You didn't answer why a re-build of the other platforms is needed.

@Longri
Copy link

Longri commented May 16, 2018

@devemux86
Copy link
Collaborator Author

Hmm and with each needed update of the library we'd need to rebuild the binary natives?

@Longri
Copy link

Longri commented May 16, 2018

Why do we have to update?
The libtess2 was also never updated!

@Longri
Copy link

Longri commented May 16, 2018

I did all native builds in #14 when forked VTM.
On Linux for all platforms and on Mac.

Which compiler did you use, on Linux?

@devemux86
Copy link
Collaborator Author

devemux86 commented May 16, 2018

That was some years ago on another machine, need to remember or rebuild.. 🙂

@devemux86
Copy link
Collaborator Author

On some Android devices there was the OOM issue.
Can build just that, test if fixes anything and at the end see the other platforms.

@Longri
Copy link

Longri commented May 16, 2018

Ok, mips/mips64 and armeabi are done, they are no longer supported by the NDK.

     [exec] Android NDK: MIPS and MIPS64 are no longer supported.    
     [exec] Android NDK: NDK Application 'local' targets unknown ABI(s): mips mips64    
     [exec] Android NDK: Please fix the APP_ABI definition in /home/default/Dokumente/gits/vtm/jni/jni/Application.mk    
     [exec] /home/default/android-ndk-r17/build/core/setup-app.mk:79: *** Android NDK: Aborting    .  Stop.

@devemux86
Copy link
Collaborator Author

Did you test the changes with regular architecture so see first if fixes the issue?

@Longri
Copy link

Longri commented May 16, 2018

Not quite so fast, please.

Now I have a point where the old native sources are compiled.

Now it's time for the new changes.

I want to pack the whole thing neatly into a PR that doesn't contain hundreds of commits. So that you can understand the changes!

@devemux86
Copy link
Collaborator Author

What I mean (and save your time!) is start with 1 simplest Android architecture, e.g. armeabi-v7a (NDK is easier than desktops), and test any changes on a device where now atlas clearly fails.
Because if there is no any improvement there is no need to spend time searching native builds. 🙂

@Longri
Copy link

Longri commented May 16, 2018

That's right, but I also wanted an easy way to compile the natives. Maybe I'll do my own PR.

We could also introduce a native version number, which is checked when loading. This ensures that the correct Native version is loaded.

@devemux86
Copy link
Collaborator Author

We could also introduce a native version number, which is checked when loading. This ensures that the correct Native version is loaded.

The natives are not provided or used separately to need custom version. They're tied to the whole library and are provided conveniently as regular versioned jars, following library publish schedule.

@Longri
Copy link

Longri commented May 18, 2018

Hi Emux, is there a function runOnMainThread()?
All platforms are Ok, except Android!
with ask for GL.MAX_TEXTURE_SIZE on non MainThread will return zero!

@devemux86
Copy link
Collaborator Author

Map has several such helper methods propagating to each platform implementations.

@Longri
Copy link

Longri commented May 18, 2018

Yes, i have seen.
But I have no Map instances on TextureAtlasUtils.
And no Gdx.app.postRunnable(task);

@devemux86
Copy link
Collaborator Author

devemux86 commented May 18, 2018

Can ask that in Activity with known methods "before" loading a theme, store the result in a static variable and then use it in TextureAtlasUtils class.

EDIT: this is for being able to quickly test it, then we can improve the implementation.

@Longri
Copy link

Longri commented May 18, 2018

I think that would be best!
Because yes I am in the MainThread but Android answers the question about GL_MAX_TEXTURE_SIZE only in the UiThread and only when the GLSurfaceView is created.

I'm trying to find a good solution.

So be patient a little longer.

@Longri
Copy link

Longri commented May 20, 2018

Okay, I've found a solution.
When Gl_SurfaceView is created, I fire an event that receives the MapActivity and only then do I call the CreateLayers method.

But I still have an OOM, which will always be possible.

If a theme has a lot of images/icons, these are completely loaded into memory to create the atlas.
I think it would be better to not load the bitmaps here but to give the BitmapPacker only the path of the bitmaps and to load, draw and destroy one bitmap after another when drawing the atlas.

@devemux86
Copy link
Collaborator Author

What's the implementation to discuss? Because many things were mentioned above.

Are there eventually any new native libs involved or we better continue with regular Java for texture atlas (by far the best solution) ?

When Gl_SurfaceView is created, I fire an event that receives the MapActivity and only then do I call the CreateLayers method.

Why exactly need all that?
e.g. there isn't any createLayers in MapsforgeActivity sample where most tests are made.

If a theme has a lot of images/icons, these are completely loaded into memory to create the atlas.
I think it would be better to not load the bitmaps here but to give the BitmapPacker only the path of the bitmaps and to load, draw and destroy one bitmap after another when drawing the atlas.

So improve TextureAtlasUtils implementation, that seems nice idea.

@Longri
Copy link

Longri commented May 20, 2018

Are there eventually any new native libs involved or we better continue with regular Java for texture atlas (by far the best solution) ?

The native code I introduced calculates only the rectangles of the items in the atlas. Everything else happens in Java.

Why exactly need all that?
e.g. there isn't any createLayers in MapsforgeActivity sample where most tests are made.

The CreateLayers() method is currently called in onCreate. The Theme will loaded at this time and on this point is the GL_SerfaceView not created.

I have changed the MapActivity to:

 protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(mContentView);

        setTitle(getClass().getSimpleName());

        mMapView = (MapView) findViewById(R.id.mapView);
        mMap = mMapView.map();
        mPrefs = new MapPreferences(MapActivity.class.getName(), this);
        GLState.event.bind(new GLState.Listener() {
            @Override
            public void onGLStateInitEvent(Event event) {
                MapActivity.this.runOnUiThread(new Runnable() {
                    @Override
                    public void run() {
                        createLayers();
                    }
                });
            }
        });
    }

    public void createLayers(){
    }

All Activities derived from MapActivity can then overwrite createLayers() and load the Theme at the right time.

@devemux86
Copy link
Collaborator Author

The native code I introduced calculates only the rectangles of the items in the atlas. Everything else happens in Java.

Why would need native code for that? How it works right now in Java?

All Activities derived from MapActivity can then overwrite createLayers() and load the Theme at the right time.

I introduced the createLayers method as a way for other samples to override, simply for all Samples to work without replicate much code.
Will see the implementation, as most users have their own ways to use the library API and need to be flexible.

@devemux86
Copy link
Collaborator Author

All Activities derived from MapActivity can then overwrite createLayers() and load the Theme at the right time.

BTW such change should only be applied to the specific Sample which demonstrates the texture atlas, that is AtlasThemeActivity.
The other samples do not use texture atlas, so shouldn't change anything in them.

@Longri
Copy link

Longri commented May 21, 2018

BTW such change should only be applied to the specific Sample which demonstrates the texture atlas, that is AtlasThemeActivity.
The other samples do not use texture atlas, so shouldn't change anything in them.

Ok, then I will take these changes back and put them only on AtlasThemeActivity.

Now the question remains whether we should log a WARNING if a user tries to use the atlas before the GL_SurfaceView was created and fall back to a minimal value (e.g. 512) or throwing a RuntimeException.

Why would need native code for that? How it works right now in Java?

Because the calculation is time consuming. Similar to the tessellate calculation, which is also used Native.
The code cannot be converted to Java because it has pointer movements, which do not exist under Java.

The C-library, which I found, also has the advantage to do the calculations in such a way, that an atlas really only has the size it needs. All other packers based on Java specify a size and use it. For the C-library, only the maximum size is specified.

@devemux86
Copy link
Collaborator Author

Need to see first if it can fix this issue, then can discuss further the implementation.
If it cannot fix this issue, then no native changes are needed in the library. 🙂

@Longri
Copy link

Longri commented May 22, 2018

PR #542 will fix this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants