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

Record mode #1582

Merged
merged 31 commits into from
Nov 24, 2020
Merged

Record mode #1582

merged 31 commits into from
Nov 24, 2020

Conversation

darkdarkdragon
Copy link
Contributor

@darkdarkdragon darkdarkdragon commented Jul 20, 2020

What does this pull request do? Explain your changes. (required)
Allows to record streams

Specific updates (required)

  • add generic URL-based OS syntax 6b7b0c7
  • add -objectStore command line argument 4cfedea
  • allow to specify OS per-stream in webhook f768a95

PR allows to specify global OS for saving streams (using -recordStore command line option) or to specify OS per-stream using webhook.
Two fields added to webhook response structure:
objectStore and recordObjectStore.
objectStore specifies URL of OS to use for stream in transcoding pipeline, and
recordObjectStore specifies URL of OS to store stream to.

Recorded streams can be played back using http://broadcaster:8935/recordings/manifestID/index.m3u8 URL.

How did you test each of these updates (required)
Units tests

Does this pull request close any open issues?
Fixes #1609
Fixes #1572

Checklist:

  • README and other documentation updated
  • Node runs in OSX and devenv
  • All tests in ./test.sh pass

iameli
iameli previously requested changes Jul 31, 2020
Copy link
Contributor

@iameli iameli left a comment

Choose a reason for hiding this comment

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

So we're going to run into some trouble with nodeId.

  1. It's not set unless -monitor=true is set, which results in an empty string being used in the S3 upload URLs and playback breaking.
  2. In on-chain mode it uses the broadcaster address... except that'll be the same across all of our production broadcasters in a region, so their JSON uploads will all collide.

I dunno what we wanna do about that exactly.

@iameli
Copy link
Contributor

iameli commented Jul 31, 2020

I tried to make a copy of this branch and accidentally deleted and recreated it, sorry if that causes problems anywhere 😂

@darkdarkdragon darkdarkdragon force-pushed the it/record-mode branch 4 times, most recently from dacab8a to a8797e7 Compare August 11, 2020 22:41
@darkdarkdragon darkdarkdragon force-pushed the it/record-mode branch 11 times, most recently from d4f93f3 to 3da898f Compare August 30, 2020 10:53
@darkdarkdragon darkdarkdragon marked this pull request as ready for review September 11, 2020 17:33
@darkdarkdragon darkdarkdragon changed the title It/record mode Record mode Sep 11, 2020
@darkdarkdragon darkdarkdragon force-pushed the it/record-mode branch 2 times, most recently from f24de4a to 4ca95f6 Compare October 16, 2020 16:51
drivers/drivers.go Outdated Show resolved Hide resolved
verification/verify.go Show resolved Hide resolved
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

Left a comment about a typo but feel free to fix during the squash.

Besides that LGTM

 - add `recordObjectStoreUrl` field to the webhook auth response - used
  as absolute URL to the segments saved to the recording
  object storage
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.

Mist timeouts with record mode enabled proposal: generic URL-based OS syntax
3 participants