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

Implement Recording V2 #1692

Merged
merged 50 commits into from
May 5, 2023
Merged

Implement Recording V2 #1692

merged 50 commits into from
May 5, 2023

Conversation

emranemran
Copy link
Contributor

@emranemran emranemran commented Apr 19, 2023

What does this pull request do? Explain your changes. (required)

Change Recording from using go-livepeer (aka. Recording V1) to using Mist (aka Recording V2).

Related design doc: Livestream Recordings Refactor

Specific updates (required)

  • Add env variable LP_API_RECORDING_SOURCE_URL to define the public bucket URL where Mist stores recordings
  • Add recordingSessionId field to the session DB object and fill it with sessionID (uuid) passed from Mist
  • Use Catalyst VOD transcoding to output the recording asset
  • Resolve recording URL (both HLS and MP4) from asset instead of statically defined source.mp4 and index.m3u8
  • Change UI to read MP4 from mp4Url (rather than converting HLS URL)

Additional comments

  • We now need to make 2 DB calls to fetch each HLS/MP4 recording path; this may have some performance implications, but I suggest to refactor/improve it in a separate PR
  • There should be also some additional changes to clean up the Recording V1 code, but I think we should do it after the deployment to prod (Added ticket for it here)

TODO before merging this PR

  • Clean up code & unit tests
  • Fix issue with session re-usage
  • Fix issue in Mist to storage Manifest file in the correct directory
  • HLS Manifest input in Catalyst

How did you test each of these updates (required)

Does this pull request close any open issues?

Screenshots (optional)

Checklist

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@emranemran emranemran requested a review from a team as a code owner April 19, 2023 00:11
@linear
Copy link

linear bot commented Apr 19, 2023

VID-136 mapic: include RecordingSessionID in PUT /setactive call

In the PUSH_REWRITE trigger handler in mapic, we need to call PUT /stream/:id/:recordingsessionid/setactive to pass alont the recording session id to Studio.

@vercel
Copy link

vercel bot commented Apr 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
livepeer-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2023 2:50pm

@emranemran emranemran removed the request for review from a team April 19, 2023 00:11
@leszko leszko marked this pull request as draft April 19, 2023 06:39
@leszko leszko changed the title setactive: accept recordingid as a new param Implement Recording V2 Apr 21, 2023
@leszko
Copy link
Contributor

leszko commented Apr 28, 2023

@victorges I think I addressed all parts related to the 1-N change. Btw. thanks for the hints about the code parts.

I think the most difficult is this isActive to avoid asset duplication for the given session. I removed all related code and used session.id as asset.id. It may be considered a hack, but on the other hand, I think this solves the issue of duplicates in the right way. I think the previous solution was actually prone to race conditions. If we don't want the asset and session to share the id, the other option is to create a new field like asset.recordingSessionId and make it unique. In theory, we could make asset.source.sessionId as unique, but I guess we already have some duplicated entries in prod. Let me know your thoughts.

Other than that, I think it works fine.

Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

@victorges Please look into this PR again.

@leszko leszko requested a review from victorges April 28, 2023 13:34
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM!! 🎉

tho the tests are still failing

packages/api/src/controllers/asset.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.test.ts Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Show resolved Hide resolved
packages/api/src/controllers/stream.ts Show resolved Hide resolved
packages/api/src/controllers/stream.ts Show resolved Hide resolved
packages/api/src/controllers/stream.ts Show resolved Hide resolved
packages/api/src/webhooks/cannon.ts Show resolved Hide resolved
Co-authored-by: Victor Elias <victor@livepeer.org>
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Addressed all the comments.

@leszko leszko merged commit 7899444 into master May 5, 2023
12 checks passed
@leszko leszko deleted the recordingv2/VID-136-137 branch May 5, 2023 05:26
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.

None yet

3 participants