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

Return actual height, width and fps for streams in /api/v1/videos #4586

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

absidue
Copy link
Contributor

@absidue absidue commented Apr 20, 2024

closes #4131

At the moment Invidious will return hardcoded data for the size, qualityLabel and fps of fields for streams, when hardcoded data is available, otherwise it just omits those fields from the response e.g. with the AV1 formats. Those issues are especially noticable when Invidious claims that 50fps streams have 60fps and when it claims that the dimensions for a vertical video are landscape. The DASH manifests that Invidious generates already use the correct information.

This pull request corrects that issue by returning the information that YouTube provides instead of hardcoded values and also fixes the long standing bug of Invidious claiming that audio streams have 30 fps.

Here are two test cases:
50/25/13fps: https://youtu.be/GbXYZwUigCM (/api/v1/videos/GbXYZwUigCM)
vertical video: https://youtu.be/hxQwWEOOyU8 (/api/v1/videos/hxQwWEOOyU8)

Originally these problems were going to be solved by the complete refactor of stream handling in #3620, but as that pull request got closed by the stale bot over a month ago and has such a massive scope that it would require a massive amount of work to complete it, I decided to open this pull request that takes a less radical approach of just fixing bugs instead of a full on refactoring.

FreeTube generates it's own DASH manifests instead of using Invidious' one, so that it can support multiple audio tracks and HDR. Unfortunately due to the missing and inaccurate information in the API responses, FreeTube has to request the DASH manifest from Invidious to extract the height, width and fps. With this pull request FreeTube could rely just on the API response, saving that extra request to the Invidious instance. It would also make it possible for FreeTube to use the vp9 streams with Invidious, which would reduce the load on the video proxies.

@absidue absidue requested a review from a team as a code owner April 20, 2024 18:12
@absidue absidue requested review from SamantazFox and removed request for a team April 20, 2024 18:12
src/invidious/jsonify/api_v1/video_json.cr Outdated Show resolved Hide resolved
src/invidious/jsonify/api_v1/video_json.cr Outdated Show resolved Hide resolved
Co-authored-by: Samantaz Fox <coding@samantaz.fr>
@SamantazFox SamantazFox added the need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something label Apr 21, 2024
@SamantazFox
Copy link
Member

It has the fun side effect of removing the fps field from audio only streams ^^
image

Also, note to self: don't forget to mention that "size": "<width>x<height>" replaces the older resolution

@absidue
Copy link
Contributor Author

absidue commented Apr 22, 2024

I didn't mean to remove the resolution field, I'll add that back, to avoid breaking API users projects.
The size field was already being used for formatStreams so I thought I would be good to use it for the adaptiveFormats too, for the sake of consistency.

quality_label += "60"
end
json.field "qualityLabel", quality_label
quality_label = "#{width > height ? height : width}p"
Copy link
Member

Choose a reason for hiding this comment

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

Something's not right here, but I can't figure out what (from the video hxQwWEOOyU8 you provided):

{
  "size": "82x144",
  "qualityLabel": "82p",
  ...
  "size": "136x240",
  "qualityLabel": "136p",
  ...
  "size": "406x720",
  "qualityLabel": "406p",
  ...
  "size": "608x1080",
  "qualityLabel": "608p",
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is another vertical video you can try, with more normal dimensions: https://youtu.be/HgEiag5zZc0 (/api/v1/videos/HgEiag5zZc0)

@SamantazFox SamantazFox added unfinished More work is needed on this PR, or on something this PR uses. and removed need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something labels Apr 24, 2024
@SamantazFox SamantazFox added need-code-review A crystal developper need to check if the code is correct. and removed unfinished More work is needed on this PR, or on something this PR uses. labels May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-code-review A crystal developper need to check if the code is correct.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] formatStreams.size parameter wrong in API response, always returns 16:9
2 participants