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

File component framerate #145

Merged
merged 9 commits into from
Feb 5, 2024
Merged

File component framerate #145

merged 9 commits into from
Feb 5, 2024

Conversation

Rados13
Copy link
Contributor

@Rados13 Rados13 commented Jan 29, 2024

Acknowledging the stipulations set forth:

  • I hereby confirm that a Pull Request involving updates to the Software Development Kit (SDK) has been smoothly merged, currently awaits processing, or is otherwise deemed unnecessary in this context.
  • I also affirm that another Pull Request, specifically addressing updates to the documentation body (commonly referred to as 'docs'), has either been successfully incorporated, is in the process of review, or is considered superfluous under the prevailing circumstances.

Docs PR
Elixir SDK PR
Python SDK PR

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Merging #145 (d946fd0) into main (f671621) will increase coverage by 0.18%.
The diff coverage is 96.96%.

❗ Current head d946fd0 differs from pull request most recent head 62f8e6f. Consider uploading reports for the commit 62f8e6f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #145      +/-   ##
==========================================
+ Coverage   87.34%   87.52%   +0.18%     
==========================================
  Files          59       59              
  Lines        1098     1114      +16     
==========================================
+ Hits          959      975      +16     
  Misses        139      139              
Files Coverage Δ
lib/jellyfish/component/file.ex 100.00% <100.00%> (ø)
lib/jellyfish/config_reader.ex 93.61% <ø> (ø)
lib/jellyfish/room.ex 86.62% <100.00%> (+0.31%) ⬆️
lib/jellyfish/room_service.ex 86.48% <100.00%> (ø)
lib/jellyfish_web/api_spec/component/file.ex 100.00% <ø> (ø)
.../jellyfish_web/controllers/component_controller.ex 95.83% <100.00%> (+0.37%) ⬆️
lib/jellyfish/component/hls/request_handler.ex 91.89% <85.71%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f671621...62f8e6f. Read the comment docs.

Copy link
Contributor

@Karolk99 Karolk99 left a comment

Choose a reason for hiding this comment

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

I think we should make framerate null or validate it when it's audio

lib/jellyfish/room.ex Outdated Show resolved Hide resolved
)

assert model_response(conn, :bad_request, "Error")["errors"] ==
"Invalid request body structure"
Copy link
Member

Choose a reason for hiding this comment

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

Why we don't expect to receive "Invalid framerate passed" here?

framerate: %Schema{
type: :integer,
description: "Framerate of video in a file. It is only valid for video track",
nullable: true,
Copy link
Member

Choose a reason for hiding this comment

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

I think framerate should only be nullable in Options. We expect Jellyfish to set it to some value.

Suggested change
nullable: true,

lib/jellyfish/component/file.ex Outdated Show resolved Hide resolved
Comment on lines +52 to +53
nullable: true,
example: 30
Copy link
Member

Choose a reason for hiding this comment

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

Actually, since we publicly declare the default is 30 (in the docs), we can do it here as well.

Suggested change
nullable: true,
example: 30
default: 30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about that as for FileComponent with audio the default will be nil.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, right 👍🏽

@Rados13 Rados13 requested review from roznawsk and Karolk99 January 31, 2024 09:22
Copy link
Member

@roznawsk roznawsk left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@Rados13 Rados13 marked this pull request as ready for review February 1, 2024 10:05
@Rados13 Rados13 force-pushed the File-component-framerate branch from d946fd0 to 62f8e6f Compare February 5, 2024 13:12
@Rados13 Rados13 merged commit b88fbd5 into main Feb 5, 2024
5 checks passed
@Rados13 Rados13 deleted the File-component-framerate branch February 5, 2024 13:22
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.

3 participants