play-co / devkit

HTML 5 game platform for browser and mobile
http://docs.gameclosure.com
622 stars 126 forks source link

All images on the screen flicker when there is a url image and device is offline. #240

Open jishnu7 opened 8 years ago

jishnu7 commented 8 years ago

Following is a simple code to reproduce the issue.

import ui.ImageView as ImageView;

exports = Class(GC.Application, function () {
  this.initUI = function () {
    // compile and open the app while device is offline.
    var view = new ImageView({
      superview: this.view,
      layout: 'box',
      x: 100,
      y: 100,
      width: 500,
      height: 500,
      backgroundColor: 'red',
      image: 'https://i.imgur.com/lLPYG3H.jpg'
    });
  };
});
collingreen commented 8 years ago

@jishnu7 this is definitely an issue on native for any imageview that doesn't have an image set. You can work around it in a few ways -- listen for online/offline changes and update your views or something like -- but I'm curious as to why you have remote images at all, particularly in a game where offline play is expected/supported.

jishnu7 commented 8 years ago

I'm guessing this is a texture manager bug. When image caching module returns a fail message, texture manager is continuously requesting to get the image, this is causing reflow on every tick.

We are using it to show user's Facebook profile picture and friends pictures. On Aug 5, 2015 11:33 PM, "Collin Green" notifications@github.com wrote:

@jishnu7 https://github.com/jishnu7 this is definitely an issue on native for any imageview that doesn't have an image set. You can work around it in a few ways -- listen for online/offline changes and update your views or something like -- but I'm curious as to why you have remote images at all, particularly in a game where offline play is expected/supported.

— Reply to this email directly or view it on GitHub https://github.com/gameclosure/devkit/issues/240#issuecomment-128093192.

jishnu7 commented 8 years ago

I have a temporary fix (in native-core). Not sending a PR since I don't know whether this will create other issues or not.

diff --git a/texture_manager.c b/texture_manager.c
index c1802a3..f94d4b3 100644
--- a/texture_manager.c
+++ b/texture_manager.c
@@ -401,7 +401,7 @@ void texture_manager_clear_textures(texture_manager *manager, bool clear_all) {
             should_use_halfsized = true;
         }

-        if (tex->loaded && (clear_all || tex->failed || overLimit)) {
+        if (tex->loaded && (clear_all || overLimit)) {
             texture_2d *to_be_destroyed = tex;
             texture_manager_free_texture(manager, to_be_destroyed);
         }
jishnu7 commented 8 years ago

Found a similar issue when device has low memory (even on devices with 2+GB RAM), after half-sizing textures, texture manager is clearing and redrawing on every tick. This is also causing same kind of flickering, but it is a different issue itself.

rampr commented 8 years ago

So, we got logs for the above, if we get a memory warning on game load or simulate memory warning on game load, flickering happens

Please find logs here -> http://pastebin.com/5fCsT3cj

It seems to go into a texture load loop and never recovers

collingreen commented 8 years ago

Agreed - it seems that some logic is missing about failed textures so it doesn't simply keep retrying them over and over forever. I've passed this on to engineers more familiar with the texture manager than me.

I'm suspicious that the overLimit flag is doing something similar if the memory never returns to a safe level - I'll have the same people look at this one as well.

rampr commented 8 years ago

@collingreen there are 2 different issues in this thread though. One flickering when loading remote images and there is not internet and the other being flickering when memory is less for which I've pasted the logs above.

collingreen commented 8 years ago

Indeed, although the first one seems to be not about internet connection but instead invalid images (you can load images on an invalid path and they flicker as well).

rampr commented 8 years ago

I've looked at the logs and looks like even after the memory returns to a safe level, the overLimit flag keeps getting triggered, which triggers clearing all textures and hence the load loop

jishnu7 commented 8 years ago

yeah @collingreen noticed that as well. forgot to comment.

rampr commented 8 years ago

We've made the following changes, this seems to solve both the cases. Please let us know your thoughts.

We've found 3 cases and tried to fix them below, in addition to the fix mentioned above:

  1. when memory_warning happens very early, the value highest sometimes is 0. This leads to a load loop because we are trying to ratchet down or increase memory against 0, which would stay 0, because MEMORY_DROP_RATE and MEMORY_GAIN_RATE are being multiplied against 0. So we set highest as 50MB (Please suggest a different value).
  2. We are increasing the max_texture bytes on two conditions,
    • when it is overLimit
    • when highest > max_texture_bytes (this is already been doing right now)
  3. overLimit calculation had a bug in texture_manager_clear_textures() function. manager->approx_bytes_to_load will reduce when textures are cleared, but that was not taken into account in the overLimit calculation.
diff --git a/texture_manager.c b/texture_manager.c
index 9df34d1..5177f40 100644
--- a/texture_manager.c
+++ b/texture_manager.c
@@ -35,6 +35,7 @@

 // Large number to start texture memory limit
 #define MAX_BYTES_FOR_TEXTURES 500000000    /* 500 MB */
+#define MIN_BYTES_FOR_TEXTURES 50000000    /* 50 MB */

 // Rate used to reduce memory limit on glError or memory warning
 #define MEMORY_DROP_RATE 0.9                /* 90% of current memory */
@@ -383,11 +384,12 @@ void texture_manager_clear_textures(texture_manager *manager, bool clear_all) {
      * 3. throw out failed textures, forcing them to reload if needed
      * 4. throw out least-recently-used textures if we exceed our estimated memory limit
      */
-    long adjusted_max_texture_bytes = manager->max_texture_bytes - manager->approx_bytes_to_load;
+    long adjusted_max_texture_bytes = 0;
     HASH_SRT(url_hash, manager->url_to_tex, last_accessed_compare);
     texture_2d *tex = NULL;
     texture_2d *tmp = NULL;
     HASH_ITER(url_hash, manager->url_to_tex, tex, tmp) {
+        adjusted_max_texture_bytes = manager->max_texture_bytes - manager->approx_bytes_to_load;
         bool overLimit = manager->texture_bytes_used > adjusted_max_texture_bytes;

         // if we reach a recently used image and still need memory, halfsize everything
@@ -395,7 +397,7 @@ void texture_manager_clear_textures(texture_manager *manager, bool clear_all) {
             should_use_halfsized = true;
         }

-        if (tex->loaded && (clear_all || tex->failed || overLimit)) {
+        if (tex->loaded && (clear_all || overLimit)) {
             texture_2d *to_be_destroyed = tex;
             texture_manager_free_texture(manager, to_be_destroyed);
         }
@@ -730,6 +732,10 @@ void texture_manager_tick(texture_manager *manager) {

     // move our estimated max memory limit up or down if necessary
     long highest = get_epoch_used_max();
+    if(highest == 0) {
+      highest = MIN_BYTES_FOR_TEXTURES;
+    }
+    bool overLimit = manager->texture_bytes_used > manager->max_texture_bytes - manager->approx_bytes_to_load;
     if (m_memory_warning) {
         m_memory_warning = false;

@@ -744,7 +750,7 @@ void texture_manager_tick(texture_manager *manager) {

         // zero the epoch used bins
         memset(m_epoch_used, 0, sizeof(m_epoch_used));
-    } else if (highest > manager->max_texture_bytes) {
+    } else if (highest > manager->max_texture_bytes || overLimit) {
         // increase the max texture bytes limit
         long new_max_bytes = MEMORY_GAIN_RATE * (double)manager->max_texture_bytes;
         TEXLOG("WARNING: Allowing more memory! Texture limit was %zu, now %zu", manager->max_texture_bytes, new_max_bytes);
@@ -891,7 +897,7 @@ void texture_manager_memory_warning() {
 void texture_manager_set_max_memory(texture_manager *manager, long bytes) {
     LOGFN("texture_manager_set_max_memory");

-    if (manager->max_texture_bytes > bytes) {
+    if (manager->max_texture_bytes > bytes && bytes > 0) {
         manager->max_texture_bytes = bytes;
     }
 }
jishnu7 commented 8 years ago

@rampr why do we need to reduce MIN_BYTES_FOR_TEXTURES to 50MB?

rampr commented 8 years ago

It is not reduced. It is a new value that is to fix the specific case when highest is 0.

rogueSkib commented 8 years ago

Hey guys, this looks pretty straightforward on first glance. Would you mind placing a pull request? I'd be happy to test out and review. Cheers!

rampr commented 8 years ago

Hi Jimmy, We'll send a Pull Request across today.

rampr commented 8 years ago

Hi @rogueSkib, Done. Sent it here -> https://github.com/gameclosure/native-core/pull/11

jishnu7 commented 8 years ago

updated the code with a background color for the image view, to make flickering more noticeable.