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

OS, heavily WIP #562

Open
wants to merge 1 commit into
base: master
from

Conversation

3 participants
@darkdarkdragon
Collaborator

darkdarkdragon commented Sep 17, 2018

mostly to decide where to fit code that puts/pulls data to/from OS
there is no any configuration yet, no negotiation on what OS to use (

I will create separate issue with all the questions I have

@darkdarkdragon darkdarkdragon requested review from j0sh and ericxtang Sep 17, 2018

@j0sh j0sh added this to Active in Weekly Sprints Sep 24, 2018

uint64 nonce = 5;
}
message OSInfo {

This comment has been minimized.

@j0sh

j0sh Oct 2, 2018

Contributor

OSes may have quite specialized credentials (eg, S3) or none at all (IPFS, local storage). How about this:

message TranscoderRequest {
...
  // indicates to T which OSes that B can use.
  repeated string supportedOS = 3;
  
  // If this is populated, contains the creds needed for S3
  S3Creds s3Creds = 16;
}

message S3Creds  {
 ... S3 credential info goes here ...
}

Can you elaborate on what the other fields are for, eg nonce, uri, broadcasterOwned? And how is OSInfo different from the TypedURI message?

@j0sh

j0sh Oct 2, 2018

Contributor

OSes may have quite specialized credentials (eg, S3) or none at all (IPFS, local storage). How about this:

message TranscoderRequest {
...
  // indicates to T which OSes that B can use.
  repeated string supportedOS = 3;
  
  // If this is populated, contains the creds needed for S3
  S3Creds s3Creds = 16;
}

message S3Creds  {
 ... S3 credential info goes here ...
}

Can you elaborate on what the other fields are for, eg nonce, uri, broadcasterOwned? And how is OSInfo different from the TypedURI message?

This comment has been minimized.

@darkdarkdragon

darkdarkdragon Oct 2, 2018

Collaborator

Yeah, don't know why I was stack with same structure for different storages...
I've added to the definition OSInfo2 message - it's better variant

@darkdarkdragon

darkdarkdragon Oct 2, 2018

Collaborator

Yeah, don't know why I was stack with same structure for different storages...
I've added to the definition OSInfo2 message - it's better variant

@@ -320,7 +336,76 @@ func (h *lphttp) ServeSegment(w http.ResponseWriter, r *http.Request) {
// grpc methods
func (h *lphttp) GetTranscoder(context context.Context, req *net.TranscoderRequest) (*net.TranscoderInfo, error) {
return getTranscoder(context, h.orchestrator, req)

This comment has been minimized.

@j0sh

j0sh Oct 2, 2018

Contributor

I think the intention here was to make testing simpler, although admittedly no tests have been written yet for this particular function. What do you think?

@j0sh

j0sh Oct 2, 2018

Contributor

I think the intention here was to make testing simpler, although admittedly no tests have been written yet for this particular function. What do you think?

This comment has been minimized.

@darkdarkdragon

darkdarkdragon Oct 2, 2018

Collaborator

I was needed to access more internal state. But now I've removed some unused code, so probably we will be able to move it back to be standalone function.

@darkdarkdragon

darkdarkdragon Oct 2, 2018

Collaborator

I was needed to access more internal state. But now I've removed some unused code, so probably we will be able to move it back to be standalone function.

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

Can you elaborate on what internal state this requires that's inaccessible? Could it be moved back now? Especially because this would make it easier to diff the changes between then and now.

@j0sh

j0sh Oct 11, 2018

Contributor

Can you elaborate on what internal state this requires that's inaccessible? Could it be moved back now? Especially because this would make it easier to diff the changes between then and now.

Show outdated Hide outdated server/rpc.go Outdated
OS implementation:
implemented 2 scenarios:
Broadcaster’s owned S3 - used as input and output storage, B gives access to own S3 to T, T puts transcoded data directly into B’s S3
Transcoder’s owned S3 - used as input and output storage, T gives access to own S3 to B, B puts original data to T’s S3, send links to S3. T puts transcoded data into S3, sends links back. B downloads transcoded data from S3 and serves them from local http
- Added ‘SaveData’ flag - tells node to save all data for each stream with separate folder, along with corresponding playlists
var liveListLev uint = 10
// CombinedVideoSource holds data in different OSes

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

Can we expand on this? For example, how CombinedVideoSource is intended to be used.

@ericxtang

ericxtang Oct 10, 2018

Member

Can we expand on this? For example, how CombinedVideoSource is intended to be used.

jid = jobId.Int64()
}
for _, os := range drivers.Storages {
session := os.StartSession(jid, string(mid), nonce)

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

I would call this something like storageSession. Otherwise someone can confuse it as a video session when it appears in the code later.

@ericxtang

ericxtang Oct 10, 2018

Member

I would call this something like storageSession. Otherwise someone can confuse it as a video session when it appears in the code later.

type videoStream struct {
streamID StreamID
osSession drivers.OSSession

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

Why do we have osSession on the videoStream level instead of on the masterSource level? Do we need the flexibility to allow video streams in the same MasterPlaylist to use different storage methods?

@ericxtang

ericxtang Oct 10, 2018

Member

Why do we have osSession on the videoStream level instead of on the masterSource level? Do we need the flexibility to allow video streams in the same MasterPlaylist to use different storage methods?

This comment has been minimized.

@darkdarkdragon

darkdarkdragon Oct 10, 2018

Collaborator

It was whole point of OS proposal - to allow different use of different OSes in different combinations. With this design we can write streams into S3, into local storage (plus saving it to disk), and into IPFS, simultaneously.

@darkdarkdragon

darkdarkdragon Oct 10, 2018

Collaborator

It was whole point of OS proposal - to allow different use of different OSes in different combinations. With this design we can write streams into S3, into local storage (plus saving it to disk), and into IPFS, simultaneously.

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

Would a master playlist have different storage methods for its media playlists? If different OSes were used, I'd think that means different master playlists, right? (However note that for most practical purposes, we must serve live playlists locally.)

For our current purposes, whether the HTTP backing store is local or S3 would depend on the current node configuration. So if the broadcaster has S3 enabled, it uses a playlist manifest with S3 URLs. Otherwise, local URLs. We don't yet have the ability to select which dynamically, only at start-up time.

One day we could add the ability to select the specific backing store to return a manifest from. Was that was the plan here? The access patterns that one would use across various OSes are still a bit murky here, eg, we'd need some way of parameterizing GetLiveHLSPlaylist to return the correct backing store, or indexing on the OS type from a higher level, and returning specific video sources for each (which would allow us to reuse the existing code for BasicVideoSource).

In any case -- the data structures seem like they should be cleaned up to make the flow more obvious and effective, since there is a lot of nested scanning here that could probably be mapped/indexed. What do you think?

@j0sh

j0sh Oct 11, 2018

Contributor

Would a master playlist have different storage methods for its media playlists? If different OSes were used, I'd think that means different master playlists, right? (However note that for most practical purposes, we must serve live playlists locally.)

For our current purposes, whether the HTTP backing store is local or S3 would depend on the current node configuration. So if the broadcaster has S3 enabled, it uses a playlist manifest with S3 URLs. Otherwise, local URLs. We don't yet have the ability to select which dynamically, only at start-up time.

One day we could add the ability to select the specific backing store to return a manifest from. Was that was the plan here? The access patterns that one would use across various OSes are still a bit murky here, eg, we'd need some way of parameterizing GetLiveHLSPlaylist to return the correct backing store, or indexing on the OS type from a higher level, and returning specific video sources for each (which would allow us to reuse the existing code for BasicVideoSource).

In any case -- the data structures seem like they should be cleaned up to make the flow more obvious and effective, since there is a lot of nested scanning here that could probably be mapped/indexed. What do you think?

glog.Error(err)
}
// XXX what if list will be longer?
fullPL, err := m3u8.NewMediaPlaylist(131072, 131072)

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

This is a limitation of the m3u8 package at the moment. What do you think about defining a global MaxSegmentsInStream, and keep track of the # of segments at the point of video ingest so we can give users the proper warning / errors? "Magic numbers" in the code generally makes me uneasy...

@ericxtang

ericxtang Oct 10, 2018

Member

This is a limitation of the m3u8 package at the moment. What do you think about defining a global MaxSegmentsInStream, and keep track of the # of segments at the point of video ingest so we can give users the proper warning / errors? "Magic numbers" in the code generally makes me uneasy...

This comment has been minimized.

@darkdarkdragon

darkdarkdragon Oct 10, 2018

Collaborator

That's why I've left comment here - it's something that's need to be addressed.
About warnings/errors - problem with them it is that we don't have good way to communicate them to user - warnings in logs gets buried very quickly, and I don't think anyone looks into it anyway...
Probably proper solution will be to create special version of MediPlayList class that will keep only start/end segments number and will generate all the segments based on these numbers and some segment's name template (good candidate for bounty?)
But for now ok, will do what you're suggested

@darkdarkdragon

darkdarkdragon Oct 10, 2018

Collaborator

That's why I've left comment here - it's something that's need to be addressed.
About warnings/errors - problem with them it is that we don't have good way to communicate them to user - warnings in logs gets buried very quickly, and I don't think anyone looks into it anyway...
Probably proper solution will be to create special version of MediPlayList class that will keep only start/end segments number and will generate all the segments based on these numbers and some segment's name template (good candidate for bounty?)
But for now ok, will do what you're suggested

if !doNotSaveMediaPlaylist {
vParams := ffmpeg.VideoProfileToVariantParams(ffmpeg.VideoProfileLookup[profile])
sid := string(streamID)
_, url, err := strm.osSession.SaveData(sid, sid+"Full.m3u8", strm.mediaFullPlayList.Encode().Bytes())

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

So now the VOD playlist has Full attached. Let's document this convention in the Readme.

@ericxtang

ericxtang Oct 10, 2018

Member

So now the VOD playlist has Full attached. Let's document this convention in the Readme.

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

Maybe make it "_full.m3u8" to make things a bit easier to read?

@j0sh

j0sh Oct 11, 2018

Contributor

Maybe make it "_full.m3u8" to make things a bit easier to read?

@@ -0,0 +1,283 @@
package core

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

This file needs tests

@ericxtang

ericxtang Oct 10, 2018

Member

This file needs tests

var Storages []OSDriver
// ExternalStorage own external storage, if configured
var ExternalStorage OSDriver

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

This is not used anywhere

@ericxtang

ericxtang Oct 10, 2018

Member

This is not used anywhere

@@ -0,0 +1,96 @@
// Package drivers abstracts different object storages, such as local, s3, ipfs

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

This file needs tests

@ericxtang

ericxtang Oct 10, 2018

Member

This file needs tests

@@ -0,0 +1,71 @@
package drivers

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

This file needs tests

@ericxtang

ericxtang Oct 10, 2018

Member

This file needs tests

@@ -0,0 +1,218 @@
package drivers

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

This file needs tests

@ericxtang

ericxtang Oct 10, 2018

Member

This file needs tests

@@ -0,0 +1,286 @@
package drivers

This comment has been minimized.

@ericxtang

ericxtang Oct 10, 2018

Member

This file needs tests

@ericxtang

ericxtang Oct 10, 2018

Member

This file needs tests

@j0sh

I'm still reading through this, but wanted to leave what I had so far.

The major bit of feedback is that I think we need a clearer separation between the manifest handling and the OS itself. Right now they are mixed up somewhat, and it's making things harder to follow. I'll think about it and review some more before making additional suggestions.

There is a significant issue with full playlists, they are not persistent across restarts, so I wonder if we should hold off on that feature for now and revisit once things are more settled. That might also give us an opportunity to integrate the feature better.

I also just noticed that there's a leak on the orchestrator side where manifests are never released after the transcode loop expires, but it's an old issue. However, this could compound if we're keeping full manifests in memory forever.

if !doNotSaveMediaPlaylist {
vParams := ffmpeg.VideoProfileToVariantParams(ffmpeg.VideoProfileLookup[profile])
sid := string(streamID)
_, url, err := strm.osSession.SaveData(sid, sid+"Full.m3u8", strm.mediaFullPlayList.Encode().Bytes())

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

Maybe make it "_full.m3u8" to make things a bit easier to read?

@j0sh

j0sh Oct 11, 2018

Contributor

Maybe make it "_full.m3u8" to make things a bit easier to read?

@@ -320,7 +336,76 @@ func (h *lphttp) ServeSegment(w http.ResponseWriter, r *http.Request) {
// grpc methods
func (h *lphttp) GetTranscoder(context context.Context, req *net.TranscoderRequest) (*net.TranscoderInfo, error) {
return getTranscoder(context, h.orchestrator, req)

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

Can you elaborate on what internal state this requires that's inaccessible? Could it be moved back now? Especially because this would make it easier to diff the changes between then and now.

@j0sh

j0sh Oct 11, 2018

Contributor

Can you elaborate on what internal state this requires that's inaccessible? Could it be moved back now? Especially because this would make it easier to diff the changes between then and now.

// Contains full list of segments.
mediaFullPlayList *m3u8.MediaPlaylist
doNotSaveMasterPlaylist bool
doNotSaveMediaPlaylist bool

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

From what I can tell, these two bools checked in negated form. Maybe it'd be clearer to change these references to saveMediaPlaylist and check if saveMediaPlaylist and so forth ? Although I feel like if we clean up the usage of AddStream, then we wouldn't need these two bools.

@j0sh

j0sh Oct 11, 2018

Contributor

From what I can tell, these two bools checked in negated form. Maybe it'd be clearer to change these references to saveMediaPlaylist and check if saveMediaPlaylist and so forth ? Although I feel like if we clean up the usage of AddStream, then we wouldn't need these two bools.

}
}
func (c *CombinedVideoSource) AddStream(manifestID ManifestID, streamID StreamID, profile string,

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

Can we derive the manifestID from the streamID here? Eg, using streamID.ManifestIDFromStreamID()?

@j0sh

j0sh Oct 11, 2018

Contributor

Can we derive the manifestID from the streamID here? Eg, using streamID.ManifestIDFromStreamID()?

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

Can we pass in the ffmpeg.VideoProfile type here rather than the string? That way it's guaranteed to be well formed. You'd have to convert it in one spot within the broadcaster before invoking AddStream, but I think that's okay; later on if we ever do real error checking there (heh), we can fail out earlier.

@j0sh

j0sh Oct 11, 2018

Contributor

Can we pass in the ffmpeg.VideoProfile type here rather than the string? That way it's guaranteed to be well formed. You'd have to convert it in one spot within the broadcaster before invoking AddStream, but I think that's okay; later on if we ever do real error checking there (heh), we can fail out earlier.

if oos != nil {
h.orchestrator.VideoSource().AddStream(mid, s, job.Profiles[i].Name, oos, true, ownOS)
}
h.orchestrator.VideoSource().AddStream(mid, s, job.Profiles[i].Name, localOSSession, false, false)

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

I'm not exactly sure why the orchestrator needs to invoke AddStream (which is a manifest manipulating operation) when all it needs is to store / serve individual segments, not playlists?

(I can see the transcoder maybe posting a full media manifest once for VOD, but I don't have a good feel yet of how this would integrate into VOD.)

@j0sh

j0sh Oct 11, 2018

Contributor

I'm not exactly sure why the orchestrator needs to invoke AddStream (which is a manifest manipulating operation) when all it needs is to store / serve individual segments, not playlists?

(I can see the transcoder maybe posting a full media manifest once for VOD, but I don't have a good feel yet of how this would integrate into VOD.)

@@ -395,7 +476,8 @@ func StartBroadcastClient(bcast Broadcaster, orchestratorServer string) error {
return nil
}
func SubmitSegment(bcast Broadcaster, seg *stream.HLSSegment, nonce uint64) (*net.TranscodeData, error) {
// XXX probably we should move this to Broadcaster interface?

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

Not sure, I like having the networking specific stuff within rpc.go with the B/O interfaces a thin and testable shim. SubmitSegment is pretty involved, esp since it uses external resources.

@j0sh

j0sh Oct 11, 2018

Contributor

Not sure, I like having the networking specific stuff within rpc.go with the B/O interfaces a thin and testable shim. SubmitSegment is pretty involved, esp since it uses external resources.

stringStreamIds[s.String()] = job.Profiles[i].Name
}
jsid := core.StreamID(job.StreamId)
mid, err := core.MakeManifestID(jsid.GetVideoID())

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

jsid.ManifestIDFromStreamID()

@j0sh

j0sh Oct 11, 2018

Contributor

jsid.ManifestIDFromStreamID()

}
// glog.Infof("%s live list after append:", sid)
// glog.Info(stream.mediaLivePlayList.String())
mseg = c.makeMediaSegment(seg, turi.UriInManifest)

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

this is done twice?

@j0sh

j0sh Oct 11, 2018

Contributor

this is done twice?

if stream.mediaFullPlayList.Count() == 0 {
stream.mediaFullPlayList.SeqNo = seg.SeqNo
}
err = stream.mediaFullPlayList.AppendSegment(mseg)

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

Can this be done in stream.addMediaSegment ?

@j0sh

j0sh Oct 11, 2018

Contributor

Can this be done in stream.addMediaSegment ?

}
res = append(res, turi)
if !stream.doNotSaveMediaPlaylist {
_, plURL, err := stream.osSession.SaveData(sid, sid+"Full.m3u8", stream.mediaFullPlayList.Encode().Bytes())

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

We need to be a little more sophisticated about this for live streams, eg transcoder restarts, job reconnects, and here the old segments would be overwritten. Maybe we should leave full manifest support for later?

@j0sh

j0sh Oct 11, 2018

Contributor

We need to be a little more sophisticated about this for live streams, eg transcoder restarts, job reconnects, and here the old segments would be overwritten. Maybe we should leave full manifest support for later?

Name: v,
Data: data,
}
turis, err := h.orchestrator.VideoSource().InsertHLSSegment(core.StreamID(streamID), tSeg)

This comment has been minimized.

@j0sh

j0sh Oct 11, 2018

Contributor

Any reason to move this outside the transcode loop? The core.TranscodeResult struct already contains URLs.

@j0sh

j0sh Oct 11, 2018

Contributor

Any reason to move this outside the transcode loop? The core.TranscodeResult struct already contains URLs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment