WebPlatformForEmbedded / WPEWebKit

WPE WebKit port (downstream)
213 stars 140 forks source link

WPE 2.38 - Thumbnail Images partially Displayed Black Before Fully Loading in Carousel slider #1379

Closed gowthami-cl-infosys closed 4 weeks ago

gowthami-cl-infosys commented 3 months ago

On RPI4 and also in sagemcom device using WPE 2.38 it is observed that thumbnail image initially displays only half of the image, with the other half appearing black after launching the app(eg:- tg4) through controler UI

Steps to reproduce:

  1. Launch the UIController using a browser
  2. Disable Resident App plugin and activate WebKitBrowser Plugin
  3. Select WebKitBrowser plugin in the UIController
  4. Set the Custom URL to https://smarttv.tg4.ie/
  5. Once the app is successfully loaded, Start scrolling the carousel slider with thumbnails in the top of the Home page.

Thumbnail image initially displays only half of the image, with the other half appearing black. After approximately 2 seconds, the full image loads properly. This issue persists when scrolling to the next thumbnail. It seems like there's a delay or inconsistency in loading the image, causing a visual glitch.

Frequency: 100%

SW version: root@m393-72180ZB-stb:/opt/logs# cat /version.txt imagename:m393-72180ZB-stb_FBT_6.1.0-rc2_20240729175055 BRANCH=6.1.0-rc2 YOCTO_VERSION=dunfell VERSION=6.1.0-rc2.07.31.24 SPIN=0 BUILD_TIME="2024-07-29 17:50:55" WPE_WEBKIT_VERSION=2.38.2+gitAUTOINC+0ac2e6f2b6-r12 Generated on Mon Jul 29 17:50:55 UTC 2024 URSR_VERSION=23.0.1

Sample videos: for RDK: https://github.com/user-attachments/assets/c1e72cae-aa83-481e-b5e1-c3c3f7ca0022 for sagemcom: https://github.com/user-attachments/assets/5e5fd395-9cd4-460c-934a-402b1a482279

Note: This issue is observed in the image which has RDK16 and WPE 2.38

suresh-khurdiya-infosys commented 2 months ago

@pgorszkowski-igalia Anything you can suggest that which module logs helpful for investigation?

pgorszkowski-igalia commented 1 month ago

@suresh-khurdiya-infosys : sorry to not comment here, I was busy with analyzing the problem and code.

So the problem can be reproduced on RPi3. I minimized the problematic test case based on the tg4 application. Additionally with some modification of the animation time (minimize it which causes generation of only two frames of animation) , I am able to reproduce the problem also on upstream (but to observe it I have to record the test case and then watch it in slowmotion/frame by frame)

So now I am investigating how to fix the problem which can be a difficult and complex task. I am consulting it with @magomez who is an expert here.

pgorszkowski-igalia commented 1 month ago

The issue is also reported on upstream: https://bugs.webkit.org/show_bug.cgi?id=280612

pgorszkowski-igalia commented 1 month ago

@suresh-khurdiya-infosys : the fix from upstream is merged in wpe-2.38

suresh-khurdiya-infosys commented 1 month ago

thanks, we will check the fix on our devices and update you.

pgorszkowski-igalia commented 1 month ago

@suresh-khurdiya-infosys : have you tested the fix on your device? can we close this ticket?

suresh-khurdiya-infosys commented 1 month ago

Thanks, Yes we will update the result soon. Testing is in progress.

suresh-khurdiya-infosys commented 4 weeks ago

@pgorszkowski-igalia Yes, fix is working in our platform as well and so far we did not see any regression issue.

suresh-khurdiya-infosys commented 3 weeks ago

@pgorszkowski-igalia In our internal code review, we received the below comment. Can you please check and update it?

m_animations.applyKeepingInternalState(futureApplicationResults, MonotonicTime::now() + 300_ms); RC: 300 ms looks like an arbitrary hardcoded value - why 300? Won't this break other animations? What if the animation was faster (or slower) ?

GraphicsLayerTransform m_layerFutureTransform; RC: We don't need this to be a class member - it's only used in 1 function, and the value is reassigned each time anyway. Remove this, and use a local variable instead.

pgorszkowski-igalia commented 3 weeks ago

GraphicsLayerTransform m_layerFutureTransform; RC: We don't need this to be a class member - it's only used in 1 function, and the value is reassigned each time anyway. Remove this, and use a local variable instead.

It cannot, look for line: https://github.com/WebKit/WebKit/commit/7f5e13b548931728d17ece82336f183d868b55e4#diff-2d250592e477f808976ab9f1e160d442983616185dc94ef27a2935d953970ae1R1411 We use m_layerFutureTransform from the parent layer.

pgorszkowski-igalia commented 3 weeks ago

m_animations.applyKeepingInternalState(futureApplicationResults, MonotonicTime::now() + 300_ms); RC: 300 ms looks like an arbitrary hardcoded value - why 300? Won't this break other animations? What if the animation > was faster (or slower) ?

The value how far we need to look in the future in case of animation highly depends on the speed of the device. In case of PC it is only 50ms, in case RPi it was 300ms. Probably it would be better to make it not hardcoded but configurable (e.g.: env var).

suresh-khurdiya-infosys commented 3 weeks ago

m_animations.applyKeepingInternalState(futureApplicationResults, MonotonicTime::now() + 300_ms); RC: 300 ms looks like an arbitrary hardcoded value - why 300? Won't this break other animations? What if the animation > was faster (or slower) ?

The value how far we need to look in the future in case of animation highly depends on the speed of the device. In case of PC it is only 50ms, in case RPi it was 300ms. Probably it would be better to make it not hardcoded but configurable (e.g.: env var).

@pgorszkowski-igalia will you be able to update the patch for the environment variable?

magomez commented 3 weeks ago

As @pgorszkowski-igalia mentioned, the time into the future we need to look depends on the hardware capabilities. A low value can make the original issue happen again, and a high value may cause a degradation in the performance of the animation because we will be rendering more tiles that needed. The 300ms is a compromise heuristic value that we determined should fit the general case. We don't think this value should be configurable unless there are strong reasons to do so. Do you have a concrete case where this value is not valid and needs to be configurable that we're trying to fix?