Newpipe: [Unified player] Heavy work on Main Thread during seeking on almost all videos

[vkay94]

Version

530f745 2020-09-08

Steps to reproduce the bug

1. Open any video (99% videos have this bug)
2. Show the controller and change the position multiple times by dragging/tapping on the seekbar
3. Wait ...

Expected behavior

The video should load shortly and start the playback and response to other actions.

Actual behaviour

The seekbar doesn't update the position for a long time. During this time the UI doesn't react to gestures ("freeze"). After a while the app crashes.

Screenshots/Screen recordings

Video demonstration: https://streamable.com/9gf4tv
The first video is the expected behavior, the second video shows the bug.

What's the reason?

During debugging I saw in the logcat that the device tries to seek to all positions which were tapped, so the doesn't discard "unneeded" positions (positions which are older than the current).

[vkay94]

I might have found a workaround for this ... kind of. It's not as smooth as the first video then (keep in mind that the video is recorded, on the device it's more fluid), but definitely faster responsive.

Just remove / comment out these lines: https://github.com/TeamNewPipe/NewPipe/blob/4b7311bafd919bdb9e6df8b8a865d14bc3908081/app/src/main/java/org/schabi/newpipe/player/VideoPlayerImpl.java#L1094
https://github.com/TeamNewPipe/NewPipe/blob/4b7311bafd919bdb9e6df8b8a865d14bc3908081/app/src/main/java/org/schabi/newpipe/player/VideoPlayerImpl.java#L1150

I don't see a reason for recreating the whole notification when the video is sought. This has a high impact on the main thread if it's occurs too often.

[Stypox]

@vkay94 please check if the apk provided in #3178 works well

[vkay94]

@Stypox It's slightly faster on the video which works well but on the others it's still freezing the UI/seekbar.

[avently]

It's slightly faster on the video which works well but on the others it's still freezing the UI/seekbar

Are you sure that the delay is caused by @Stypox's notifications? Please, try to measure using logcat timestamps or Android Profiler

[vkay94]

@avently I think so. Probably not the actual recreation as I looked into the code of the pr (forceRefresh-parameter) but it has an impact. Maybe it's the source. I don't know :(

What I did during recording:

• Opened this video https://www.youtube.com/watch?v=b13xnFp_LJs
• Paused the video and tapped multiple times on different places of the seekbar

Edit: Or it's more likely the changeState() method which is called often. I'm not that good at analyzing it in this view :')

[avently]

@Stypox probably in case of buffering we can drop updating a notification, what do you think? It's not important event and can happen often

[Stypox]

probably in case of buffering we can drop updating a notification, what do you think?

Unfortunately we can't drop it completely, since a notification button changes icon alongside the buffering state, but maybe we could implement a check that prevents useless updates if the state in reality did not change. I'll not include this in #3178 though, since it is not critical and not a new bug (it has always existed). I will get to it later.

[vkay94]

@Stypox @avently I figured out something this moment. This specific lag seems unrelated to the notifications since disabling improves the experience but doesn't eliminate it completely.

The reason seems to lie in the extractor or more likely on the client handling of the channels data (specific data from the channel which is added to StreamInfo or similar).
For the channel Studio Yuraki (https://www.youtube.com/channel/UCMTPj3w6L0hol6KHu9DHzQA) for example all videos don't have this bug whereas all videos from the Apple channel have this bug. There are more channels where this applies too.
So you can remove this issue from the To Do in Notification @Stypox ;)

I try to look into it but it could take some time since I haven't got much time currently.

[Stypox]

@vkay94 then it's probably a codec-related issue, since the extractor only runs once (when showing info) and provides the stream url to play.

[vkay94]

@Stypox I checked the codecs some days ago and they were identical. But I found the problem (it's so dumb):
The problem lies in handling of the extractor data as expected. The thumbnail url which is loaded by the Notification isn't unified in this call:

https://github.com/TeamNewPipe/NewPipe/blob/b69e477ecdf576d0406c0de87db3c8a2866ce741/app/src/main/java/org/schabi/newpipe/player/BasePlayer.java#L469

The extractor passes full imageurls; some of them are maxresdefault (1280x720), others are hqdefault (336x188). So when the Imageloader loads twice this max res image in every notification reset there is huge work on the main thread.

I modified the handling by unifying it to hqdefault and it works fine now for every video. So you should build a similar check into your PR, it will improve it for sure.

[Stypox]

@vkay94 unfortunately that's not the way to go, since we need the high res thumbnails to show them in the video details page. @wb9688 and @B0pol were planning to add an Image class, which would provide thumbmails at different resolutions, only then this issue can be solved. Thank you for the investigation!

[vkay94]

This image class sounds great, it'd be handy for the list items (history etc) too which are updated more often in the unified player builds than before.