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

Add durationPerFrame to Frameset #507

Merged
merged 1 commit into from
Jan 14, 2021

Conversation

vkay94
Copy link
Contributor

@vkay94 vkay94 commented Jan 4, 2021

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

This PR adds durationPerFrame field to the existing Frameset class. This property is necessary to calculate the correct frame depending on the timestamp. Before there was no way to crop the correct frame bounds from the storyboard ("framesPerPageX x framesPerPageY"-matrix).

Short example with this change to calculate the correct frame coordinates (pseudo code):

-- Goal: get frame bounds for 3:25:500 (= 180.500 milliseconds)´
-- Given: storyboard-Urls in 5x5-matrix, duration per frame = 5.000, frame height = 160 and frame width = 90

=> 180.500 / 5.000 = 36.1 => 37. Frame
=> (37 - 1 * 25) = 12 => 12. frame in 2. storyboard
=> (12 - 2 * 5) = 2 => 3. row, 2. column => row index = 2, column index = 1

=> Bounds / rectangle of the frame at the position: left top (x, y) = (0 + 1 * 160 = 160, 0 + 2 * 90 = 180)
=> (left, top, right, bottom) = (160, 180, 320, 270)

@TobiGr
Copy link
Contributor

TobiGr commented Jan 6, 2021

Thank you. Could you put that info into to the class JDoc so the class is easier to use?

@vkay94
Copy link
Contributor Author

vkay94 commented Jan 6, 2021

@TobiGr Okay, I'll add it. Actually, I'd prefer an open/public default implementation - something like

public int[] getFrameBoundsAt(long timestamp)

where the int-array returns left, top, right, bottom in that order. It would reduce code on the client-side.

I just checked the other services and PeerTube doesn't support frames and media.ccc.de uses the same logic as YouTube (grid) (but here you've got to load an additional vtt -file to get the count and duration per frame).

Url: https://static.media.ccc.de/media/events/rc3/150-8e6fd90f-f358-518f-95dc-19dbec1dcaea.thumbnails.vtt
Contents: https://pastebin.com/tSgFNBrV

What do you think?

@TobiGr
Copy link
Contributor

TobiGr commented Jan 6, 2021

Sounds good.

@vkay94 vkay94 force-pushed the add-interval-to-stream-frames branch from 65caff4 to 3d9fbc8 Compare January 6, 2021 16:44
@vkay94
Copy link
Contributor Author

vkay94 commented Jan 6, 2021

I've added the default implementation. It should work correctly (even if the "testing" has basically been with the eyes :') )

@vkay94 vkay94 force-pushed the add-interval-to-stream-frames branch from 3d9fbc8 to b727c3e Compare January 6, 2021 18:00
@Stypox Stypox requested a review from TobiGr January 14, 2021 17:49
Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be merged after this small change

@vkay94 vkay94 force-pushed the add-interval-to-stream-frames branch from b727c3e to 11dcfe6 Compare January 14, 2021 19:01
@TobiGr TobiGr merged commit cc51c5f into TeamNewPipe:dev Jan 14, 2021
@vkay94 vkay94 deleted the add-interval-to-stream-frames branch January 14, 2021 22:30
@TobiGr TobiGr mentioned this pull request Jan 18, 2021
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants