Music app issues when playing video in Firefox on Android

mruss77 commented 3 years ago

What Happened?

The Music app stops responding to screen and button inputs, or sometimes InfiniTime crashes

What should happen instead?

The Music app should still respond to button and screen input and InfiniTime should not crash

Reproduction Steps

Using Firefox on Android with Gadgetbridge:

Go to a page with an embedded video, e.g.: https://boingboing.net/2021/11/10/kitty-is-an-early-realistic-computer-animation-from-1968.html
Start playing the video
On PineTime, open the Music app

The music app shows the video title and URL (scrolling since they are long) but no longer responds to button or screen presses. The watch must be restarted by holding the button. Or sometimes InfiniTime crashes altogether and reboots.

More Details

I'm on Android 12. The issue only happens with Firefox; watching the same videos in Chrome or the YouTube app does not impact the Music app. It seems these apps also don't send the video metadata to the watch (or at least the Music app doesn't display it).

I see the same issue on other sites with embedded videos, and not just YouTube-hosted videos.

This may be considered low priority since Chrome is the default browser and seems to be used by almost all Android users. I don't generally use the watch to control embedded video playback, but if I accidentally open Music while video is playing, I'm stuck having to reboot the watch.

Version

72900ca

Companion App

Gadgetbridge v0.62.0

Avamander commented 3 years ago

InfiniTime probably shouldn't freeze because of this, but Gadgetbridge should avoid causing this as well.

JF002 commented 2 years ago

I could easily reproduce the issue and analyze it : DisplayTask is not looping anymore because it's stuck in lv_task_handler. It seems that processing the refresh of the screen when displaying very long text takes too much time. If this time is > than the refresh time, lv_task_handler will simply loop again to refresh the screen again. In this case, it's looping indefinitely because the refresh takes too much time.

According to this, this is an expected behavior in LVGL7 which was improved in LVGL8 (see last comment.

While analyzing this issue, I found 2 new issues :

The button is not taken into account when DisplayTask consumes too much CPU time. This is probably because the TIMER task has a lower priority than DisplayTask
MusicService does not limit the size of the data it receives from BLE. Instead, it simply takes all the data from BLE and copy it into std::string, which does dynamic allocation on the heap. I think this should be avoided. Instead, we should limit the number of characters (to 30 or 40, for example) and copy them into a statically allocated buffer. This might prevent heap over-allocation and memory allocation issues.

I can see 2 options for this issue:

Fix LVGL to ensure it won't enter into an infinite refresh loop of the processing takes too much time (> 20ms)
Limit the number of characters for the artist/track name in MusicService to ensure the process time won't be too long. A max value of 30 or 40 characters seems to work fine (refresh time between 40 and 180ms).

@Avamander @Riksu9000 Do you have any opinion about this?

Riksu9000 commented 2 years ago

I had a similar issue when I converted to using LVGL tasks for Refresh. This is what I wrote in #497

LVGL for some reason runs all tasks with higher priority again after a task with a lower priority has finished. The screen draw task is LV_TASK_PRIO_MID, so if the refresh task priority is lower than mid, it will redraw the screen. If the screen draw takes longer than the refresh period, the refresh task will be run again, which will run the screen draw again, resulting in an infinite loop and lv_task_handler() never returns. Because of this, the refresh task must be at least LV_TASK_PRIO_MID.

This may not be 100% accurate, since the behaviour is a bit confusing and not well documented. Nevertheless setting the priority to low in Motion app for example produces a very similar issue as seen here.

There's also the Dice app PR #565, which allows rolling up to 100 d100 dice, which produces a very long label which also lags the screen, but that doesn't cause any problems either.

I tried removing the Refresh task in Music, and it still got stuck. Then I simply hard coded two labels to contain very long text and that produces this issue. The issue is simply that LVGL can't handle drawing two long scrolling labels at the same time.

I would really prefer to upgrade to LVGL 8, since we seem to be stumbling across more and more issues with version 7, but I can see that it wouldn't be so simple. If we are going to stick with v7 for a while longer, it's probably worth patching this issue, but if we instead were to start focusing on upgrading, then a simple fix to limit the displayed characters would be the best option in my opinion.

JF002 commented 2 years ago

Thanks for this analysis, @Riksu9000 ! I agree with you, upgrading to LVGL8 is probably the best solution. However, it'll require quite some work, and I worry about memory usage increases... Someone would have to try it :)

According to the code, the refresh task is already set to LV_TASK_PRIO_MID. I'm not sure why, but increasing the refresh period did not change anything.

In any case, I think that we should limit the size of the strings copied from the BLE notification. Heap is not infinite, and this could potentially prevent a few strange bugs when the companion app sends really big strings as track or album name.

This would also provide us a short term solution for this issue.

I also tested these changes in lv_task.c:

Index: src/lv_misc/lv_task.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/lv_misc/lv_task.c b/src/lv_misc/lv_task.c
--- a/src/lv_misc/lv_task.c (revision 23430cf20e32294549fff9b2879a9466dacc19bb)
+++ b/src/lv_misc/lv_task.c (date 1647285586769)
@@ -81,6 +81,9 @@

     uint32_t handler_start = lv_tick_get();

+    uint32_t size = _lv_ll_get_len(&LV_GC_ROOT(_lv_task_ll));
+    uint32_t count = 0;
+
     /* Run all task from the highest to the lowest priority
      * If a lower priority task is executed check task again from the highest priority
      * but on the priority of executed tasks don't run tasks before the executed*/
@@ -153,6 +156,12 @@
             }

             LV_GC_ROOT(_lv_task_act) = next; /*Load the next task*/
+
+           if(count >= size) {
+             end_flag=true;
+             LV_GC_ROOT(_lv_task_act) = NULL;
+           }
+            count++;
         }
     } while(!end_flag);

And they seem to work fine, even with strings longer than 40 characters. It simply counts the number of tasks that are already present in the list, and limit the number of iterations to that number of tasks. This way, it ensures that it won't loop forever is tasks are added while it's executing other tasks.

I did a few test and couldn't see any negative side effects (yet).

JF002 commented 2 years ago

This issue should be fixed with #1036 & #1037. I'll keep this issue open as I would like to see if we can find a longer-term solution in LVGL.

InfiniTimeOrg / InfiniTime