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

Stream url list #263

Merged
merged 16 commits into from
Feb 28, 2024
Merged

Stream url list #263

merged 16 commits into from
Feb 28, 2024

Conversation

elv-zenia
Copy link
Contributor

Add StreamListUrls to return a list of pre-allocated URLs and their active/inactive status.

Example response

{
  mpegts: [
    {
      url: 'udp://...',
      active: true
    },
    {
      url: 'udp://...',
      active: false
    },
    {
      url: 'udp://...',
      active: false
    },
    {
      url: 'udp://...',
      active: true
    }
  ],
  rtmp: [
    {
      url: 'rtmp://...',
      active: false
    },
    {
      url: 'rtmp://...',
      active: true
    }
  ],
  srt: [
    {
      url: 'srt://...',
      active: false
    },
    {
      url: 'srt://...',
      active: false
    }
  ]
}

Copy link
Contributor

@elv-serban elv-serban left a comment

Choose a reason for hiding this comment

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

The logic looks good.

Noting here that in the fabric API v2 we can replace the metadata GETs for each object with a single 'fields' query across the tenancy (so we don't need to optimize this area at the moment, in view of replacing later with the v2 query)

- Fail if not stopped state
- Remove StreamDeactivate as it is now a duplicate method
- Generate new token if token meta fails
@elv-zenia
Copy link
Contributor Author

Added the changes for StreamStopSession (#262)

Comment on lines 829 to 831
if(!edgeMeta) {
let response = await this.EditContentObject({
libraryId: libraryId,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should create the new write token in all cases.
This is because the edge write token has a ton of metadata in it - as created by the live recorder (the list of all video and audio parts and metadata for each part, ...).

I think we should do nothing when edge_meta doesn't work.
And if it does work then we can check the status, like you already to, and then discard/delete the edge write token (though I am not sure if we have a client method to discard a write token)

Then just open the new write token in all cases and write the status fields, then finalize and publish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't look like there's a client method to discard a write token. Is there a contract method for this?

@elv-zenia
Copy link
Contributor Author

@elv-serban Updated the code. The edge write token gets removed from the object's metadata; it just needs to get discarded if there's a way.

@@ -767,8 +767,7 @@ exports.StreamStartOrStopOrReset = async function({name, op}) {
};

/**
* Stop the live stream session and close the edge write token.
* Not implemented fully
* close the edge write token for the live stream session.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can say 'close the edge write token ...' and make the stream object inactive.

src/client/LiveStream.js Outdated Show resolved Hide resolved
src/client/LiveStream.js Outdated Show resolved Hide resolved
src/client/LiveStream.js Outdated Show resolved Hide resolved
src/client/LiveStream.js Show resolved Hide resolved
@elv-zenia
Copy link
Contributor Author

@elv-serban Updated to address comments. One thing I'm not sure about. The response from terminating a stream is below. Should the edge_write_token be the one found in the metadata (if found) that was deleted?

return {
      fin,
      name,
      edge_write_token: <token>,
      state: newState
    };

Copy link
Contributor

@elv-serban elv-serban left a comment

Choose a reason for hiding this comment

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

Looks good!

stream.sources &&
stream.sources.default &&
stream.sources.default["."] &&
stream.sources.default["."].container ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably cleaner to use optional chaining for this

stream?.sources?.default?.["."]?.container

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'm hesitant to use optional chaining in client-js because I don't think all apps that use it support it. I think there are a few Webpack 4 projects

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, right this is the client. Fair point, no optional chaining


if(edgeWriteToken === undefined || edgeWriteToken === "") {
if(metaEdgeWriteToken === undefined || metaEdgeWriteToken === "") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

!metaEdgeWriteToken? or does it matter if it's null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok changed this

@elv-zenia elv-zenia merged commit 7c9b512 into develop Feb 28, 2024
@elv-zenia elv-zenia deleted the stream-url-list branch February 28, 2024 22:07
@elv-zenia
Copy link
Contributor Author

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