Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Check for IO errors when loading binary shader #745

Merged
merged 1 commit into from
Jan 14, 2015
Merged

Conversation

ljbade
Copy link
Contributor

@ljbade ljbade commented Jan 12, 2015

This fixes #740

I had to add checks for IO errors (I thought iostream threw on errors).

I also put in the sanity check code from android-mason for good measure.

@ljbade
Copy link
Contributor Author

ljbade commented Jan 12, 2015

I just found out that you can actually enable C++ exceptions with http://en.cppreference.com/w/cpp/io/basic_ios/exceptions

@jfirebaugh Would this be a better way to handle errors?

@kkaefer
Copy link
Member

kkaefer commented Jan 12, 2015

Can we use our caching infrastructure instead of writing directly to disk? E.g., we could introduce a cache:// scheme that only reads/writes to the cache, but doesn't attempt to load anything from disk. If it can't find the URL in cache, it just returns a not found error and the requesting application can generate the resource manually and then store it into the cache.

@ljbade
Copy link
Contributor Author

ljbade commented Jan 12, 2015

I suppose we could do that. Does the caching infrastructure support overwriting?

Since binary shader depends on GL state combinations, loading different maps/areas could trigger more information to be included in the binary shader each time the app is closed.

@ljbade
Copy link
Contributor Author

ljbade commented Jan 12, 2015

@kkaefer Perhaps I should cut a new ticket for that? In the mean time this pull request will at least fix a crash that was holding me up on working on another bug.

@jfirebaugh
Copy link
Contributor

Yeah, I generally prefer exceptions to manual error checking. In this case, I think the code should also handle any other exceptions (e.g. from MBGL_CHECK_ERROR) by abandoning the cached shaders, so it's probably easiest if the whole block is surrounded with try { } catch (...) { }.

@ljbade ljbade force-pushed the fix-binary-shader branch 2 times, most recently from d4be23c to 4caa48d Compare January 13, 2015 15:22
@ljbade
Copy link
Contributor Author

ljbade commented Jan 13, 2015

@jfirebaugh converted to exceptions

Also fixed the missing call to map::terminate which was causing havok with shader destructor. I think this might have been causing some other crashes on Android too.

Can you review and merge if all is OK?

@ljbade ljbade force-pushed the fix-binary-shader branch from 4caa48d to 6e3fddc Compare January 13, 2015 15:47
@ljbade
Copy link
Contributor Author

ljbade commented Jan 13, 2015

Also fix rare crash when HTTP request finishes after Map termination begins.

Log::Error(Event::Shader, "Loading binary shader failed!");

// Delete the bad file
std::remove(binaryFileName.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which std::remove is this? I'm surprised a one-argument call compiles.

@ljbade ljbade force-pushed the fix-binary-shader branch from 6e3fddc to 8cca0a2 Compare January 13, 2015 21:35
ljbade pushed a commit that referenced this pull request Jan 14, 2015
Check for IO errors when loading binary shader
@ljbade ljbade merged commit f76bd8a into master Jan 14, 2015
@ljbade ljbade deleted the fix-binary-shader branch January 14, 2015 01:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cached shader loader keeps reading garbage
3 participants