Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 tasks done
Domiiniik opened this issue Apr 28, 2023 · 12 comments
Closed
3 tasks done

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

Domiiniik opened this issue Apr 28, 2023 · 12 comments
Labels
Bug report Something isn't working

Comments

@Domiiniik
Copy link

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

  • I have searched the existing issues and this is a new and no duplicate or related to another open issue.
  • I have written a short but informative title.
  • I filled out all of the requested information in this issue properly.
@Domiiniik Domiiniik added the Bug report Something isn't working label Apr 28, 2023
@LisoUseInAIKyrios
Copy link
Contributor

LisoUseInAIKyrios commented Apr 28, 2023

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.

@LisoUseInAIKyrios LisoUseInAIKyrios added the Waiting on author Further information is requested label Apr 28, 2023
@SodaWithoutSparkles
Copy link
Contributor

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
Copy link
Contributor

LisoUseInAIKyrios commented Apr 28, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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: #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
Copy link
Contributor

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 bug: hide-video-buttons: incorrect buttons are being hidden (Share/Create, clip and thanks) #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.

@LisoUseInAIKyrios LisoUseInAIKyrios removed the Waiting on author Further information is requested label Apr 28, 2023
@SodaWithoutSparkles
Copy link
Contributor

SodaWithoutSparkles commented Apr 28, 2023

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
Copy link
Contributor

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
Copy link
Contributor

Fixed with ReVanced/revanced-integrations#374

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

@SodaWithoutSparkles
Copy link
Contributor

SodaWithoutSparkles commented Apr 28, 2023

@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:

  • API changes to add dev channel
  • manager changes to accept the dev channel

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

@LisoUseInAIKyrios
Copy link
Contributor

LisoUseInAIKyrios commented Apr 28, 2023

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
Copy link
Member

Moving to ReVanced/revanced-patches-template#1256

@oSumAtrIX oSumAtrIX transferred this issue from ReVanced/revanced-patches-template Dec 14, 2023
@oSumAtrIX oSumAtrIX transferred this issue from another repository Dec 14, 2023
Slenderman00 pushed a commit to Slenderman00/revanced-patches-grindr that referenced this issue Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug report Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants