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 support for standard <video> and <audio> formats #1054

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Feb 5, 2023

Related Issue

resolves #1038

Summary of Changes

  1. Add support for standard <video> and <audio> formats
  2. Added one-off support for Content-Length header which was needed by Safari (will make more robust as part handle merging additional Request / Response instance properties #1048)
  3. Found a missing case of needing to "forward" request headers for the serve command

TODO

  1. Add support for standard <audio> formats
  2. Handle conflicts with "competing" extensions like .ts (TypeScript and Apple Live Streaming) or .wav and OGG, since it could be both . Right now we break on first come, first serve 🤔
  3. How to best handle Safari for generic media container formats? - solved through above solution
    • ?type=audio|video?
  4. Create discussion for solving the overlapping extensions issues (e.g. .ts)
    https://developer.apple.com/library/archive/documentation/AppleApplications/Reference/SafariWebContent/CreatingVideoforSafarioniPhone/CreatingVideoforSafarioniPhone.html#//apple_ref/doc/uid/TP40006514-SW6
  5. Documentation / callout about asset handling on Safari - N / A
  6. Although it plays fine, getting this error on the NodeJS side when loading any media file in Safari - deferred to handle merging additional Request / Response instance properties #1048
    Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:217:20)

@thescientist13 thescientist13 added CLI feature New feature or request labels Feb 5, 2023
@thescientist13 thescientist13 changed the title add support for standard video formats add support for standard <video> and <audio> formats Feb 5, 2023
@thescientist13 thescientist13 linked an issue Feb 5, 2023 that may be closed by this pull request
3 tasks
.gitattributes Outdated
*.flv binary
*.mp4 binary
*.ts binary
Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, will this convert typescript files too? 🤔


return new Response(body, {
headers: new Headers({
'Content-Type': `image/${contentType}`
Copy link
Member Author

Choose a reason for hiding this comment

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

Whoa... this is really wrong 😬

Wonder how it's still working though?? 🤨

@thescientist13 thescientist13 added the question Further information is requested label Feb 6, 2023
@thescientist13 thescientist13 force-pushed the feature/issue-1038-standard-audio-and-video-formats branch from 25a277e to 7e6604b Compare February 7, 2023 02:25
@thescientist13 thescientist13 self-assigned this Feb 19, 2023
@thescientist13 thescientist13 marked this pull request as ready for review February 19, 2023 17:31
@thescientist13
Copy link
Member Author

thescientist13 commented Mar 1, 2023

So maybe I have been over thinking this or misinterpreting some of the data I read, but it seems like .mp4 and .3pg are both video (container) formats, so maybe we don't need any of this generic container format handling.

This would just leave .ogg then, which does seem to be a container format and seems like we can just use a generic content type instead of trying to "sniff" it out
https://developer.mozilla.org/en-US/docs/Web/HTTP/Configuring_servers_for_Ogg_media

AddType audio/ogg .oga
AddType video/ogg .ogv
AddType application/ogg .ogg

That said, the Sec-Fetch-Dest header was still an interesting find, but maybe a too prescriptive a solution.

@thescientist13 thescientist13 removed the question Further information is requested label Mar 4, 2023
@thescientist13 thescientist13 mentioned this pull request Mar 11, 2023
22 tasks
@thescientist13 thescientist13 force-pushed the feature/issue-1038-standard-audio-and-video-formats branch from 58b06c0 to 7157728 Compare March 11, 2023 15:04
@thescientist13 thescientist13 merged commit 58c0e92 into release/0.28.0 Mar 11, 2023
@thescientist13 thescientist13 deleted the feature/issue-1038-standard-audio-and-video-formats branch March 11, 2023 15:20
thescientist13 added a commit that referenced this pull request Apr 9, 2023
* add support for standard video formats

* fix content type in video response

* restore content length header for Safari support

* remove standard .ts video format support

* adding audio resource support and adding test cases

* misc refactor

* refactor audio and video files according to better understanding of container formats

* clean up TODO comment
thescientist13 added a commit that referenced this pull request Apr 9, 2023
* add support for standard video formats

* fix content type in video response

* restore content length header for Safari support

* remove standard .ts video format support

* adding audio resource support and adding test cases

* misc refactor

* refactor audio and video files according to better understanding of container formats

* clean up TODO comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support web standard <audio> and <video> formats
1 participant