search

ReVanced / revanced-patches

🧩 Patches for ReVanced
https://revanced.app
GNU General Public License v3.0
2.15k stars 247 forks source link

bug: Subtitles placed incorrectly on the top of the video #1079

Closed Domiiniik closed 1 year ago

Domiiniik commented 1 year ago

Type

Cosmetic

Bug description

Subtitles in reVanced are placed on top of the video instead of at the bottom as they should. Link to the video: https://youtu.be/HTnQRV_L32c ReVanced Screenshot_20230428_063027_YouTube Stock Youtube: Screenshot_20230428_063013_YouTube

Steps to reproduce

~

Relevant log output

~

Screenshots or videos

No response

Solution

No response

Additional context

No response

Acknowledgements

LisoUseInAIKyrios commented 1 year ago

An additional fix for subtitles was made a few days ago, and was just released today as 2.171.0

Try again using the latest release.

SodaWithoutSparkles commented 1 year ago

I patched just now using 2.171.0. This issue is still there, but it seems that this issue only affects the first video. Once you watched another video, the subtitle position returns to the bottom. Watch the first video again and the subtitles are normal again.

Steps to reporduce:

  1. Force-stop revanced and clear its cache
  2. Launch revanced and watch a video that you for-certain knows it has subtitle. Example: one from LTT
  3. Expect to see subtitles are at the top
  4. Watch another video that you also knows that there are subtitles
  5. Expect to see subtitles are in the bottom
  6. Go back and watch the same video from step 2
  7. Expect to see the subtitles goes back to the bottom again
LisoUseInAIKyrios commented 1 year ago

Post links to the videos you used, or post a screen recorder video showing the problem.

The subtitles are always correct for me (even after force stopping and relaunching)

SodaWithoutSparkles commented 1 year ago

While I am here, do you want some kind of debug log? I have adb access to that device. I don't know what I should grep for tho.

LisoUseInAIKyrios commented 1 year ago

I was able to reproduce, and the subtitles did appear in the wrong location.

It appears that sometimes when the video first starts, YouTube sends a single non default subtitle position in the subtitle stream, and this triggers the logic used to fix: https://github.com/revanced/revanced-patches/pull/1975

revanced: SpoofSignatureVerificationPatch: video: HTnQRV_L32c spoof: true ap:34 ah:50 av:95 vs:true sd:false
revanced: SpoofSignatureVerificationPatch: Non default subtitles found. Using existing settings without replacement.
revanced: SpoofSignatureVerificationPatch: video: HTnQRV_L32c spoof: true ap:9 ah:20 av:0 vs:true sd:true
LisoUseInAIKyrios commented 1 year ago

I see two possibly actions:

  1. change the passthru logic to ignore the first call with non default window positions (should fix the issue of YouTube sending one non-default subtitle setting)
  2. remove the fix done in https://github.com/revanced/revanced-patches/pull/1975. This will break subtitles that use multiple positions on screen (and one of the subtitles is default).

For now I think option 1 is the best to try.

SodaWithoutSparkles commented 1 year ago

Or use both the first and the second call to determine if the fix was needed? If the first call is non-standard then check the second call as well.

Wouldn't option 1 cause the first subtitle line to be dropped/hidden/cause undocumented behaviours?

LisoUseInAIKyrios commented 1 year ago

Option 1 does the check first and second subtitle behavior you are describing. The subtitles are a stream during the video playback, and each subtitle text has it's own position.

The fix I am describing, is a change to the 'pass thru mode' that was added in 1975. It would enable pass thru mode (do not alter the subtitle window position), only if more than 1 non default subtitle position is encountered.

LisoUseInAIKyrios commented 1 year ago

Fixed with https://github.com/revanced/revanced-integrations/pull/374

Either use the dev branch, or (for now) ignore the incorrect subtitle position for the first video opened after app launch.

SodaWithoutSparkles commented 1 year ago

@LisoUseInAIKyrios Do you also contribute in the RV manager project? It would be nice to be able to use the dev channel on manager.

Edit: Ah it might not be that easy. RVM probably gets the patches from https://releases.revanced.app/tools API, so changing this either requires:

Or RVM can get the changes from github's API directly...?

LisoUseInAIKyrios commented 1 year ago

I agree it would be awesome if the manager could use the dev release. But no, I don't contribute much to the manager (not familiar with the Flutter language it uses).

Could open a feature request in the manager repo, as I don't think there is a request for that just yet.

oSumAtrIX commented 1 year ago

Moving to ReVanced/revanced-patches-template#1256