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

Populate recordingURL when source playback is ready #1847

Merged
merged 3 commits into from
Aug 19, 2023
Merged

Conversation

mjh1
Copy link
Contributor

@mjh1 mjh1 commented Aug 16, 2023

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

Populate recordingURL when source playback is ready

Specific updates (required)

  • Populate recordingURL as soon as source playback is available
  • Refactor the function to simplify and set the recording v2 status based on the recording vod asset
    How did you test each of these updates (required)
    Tested in staging.

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.

@vercel
Copy link

vercel bot commented Aug 16, 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 Aug 19, 2023 8:26pm

@@ -447,9 +450,6 @@ export async function getRecordingFields(
const isUnused = !session.lastSeen && session.createdAt < readyThreshold;

const recordingStatus = isReady ? "ready" : isUnused ? "none" : "waiting";
if (!isReady && !forceUrl) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@victorges do you know what the forceUrl param was used for? It's not used anywhere other than these lines I removed. I removed them because I want to just check for the presence of a playbackURL rather than checking the recording status field (to allow us to get source playback through the recordingURL field).

Copy link
Member

Choose a reason for hiding this comment

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

This is for the API to return a recordingURL even if it might not have been complete yet. I think this was more useful on the recording v1 to be honest, where recordings were playable even while the stream was live and we only had an artificial wait on the API side to make sure the stream was finished.

I think it would be a noop in this flow here, since the getRecordingUrls would return undefined anyway and we wouldn't be able to force URLs. Does that seem correct? If so, then it should be completely fine removing this.

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 with a suggestion for an additional change for the recordingStatus

@@ -447,9 +450,6 @@ export async function getRecordingFields(
const isUnused = !session.lastSeen && session.createdAt < readyThreshold;

const recordingStatus = isReady ? "ready" : isUnused ? "none" : "waiting";
if (!isReady && !forceUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

This is for the API to return a recordingURL even if it might not have been complete yet. I think this was more useful on the recording v1 to be honest, where recordings were playable even while the stream was live and we only had an artificial wait on the API side to make sure the stream was finished.

I think it would be a noop in this flow here, since the getRecordingUrls would return undefined anyway and we wouldn't be able to force URLs. Does that seem correct? If so, then it should be completely fine removing this.

packages/api/src/controllers/stream.ts Show resolved Hide resolved
For recording V2 we can just base the recording status on the asset status
@mjh1 mjh1 marked this pull request as ready for review August 18, 2023 13:15
@mjh1 mjh1 requested a review from a team as a code owner August 18, 2023 13:15
@mjh1 mjh1 requested a review from victorges August 18, 2023 13:15
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

packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
@mjh1 mjh1 merged commit 889c662 into master Aug 19, 2023
11 checks passed
@mjh1 mjh1 deleted the mh/recording-url branch August 19, 2023 21:03
gioelecerati added a commit that referenced this pull request Aug 21, 2023
mjh1 added a commit that referenced this pull request Aug 23, 2023
* Populate recordingURL when source playback is ready

* Simplify recording status logic

For recording V2 we can just base the recording status on the asset status

* Fix logic
mjh1 added a commit that referenced this pull request Aug 24, 2023
* Populate recordingURL when source playback is ready

* Simplify recording status logic

For recording V2 we can just base the recording status on the asset status

* Fix logic
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