-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: Make mapic stateless #221
Conversation
8b5b8c3
to
dfeb437
Compare
info.mu.Unlock() | ||
return | ||
} | ||
glog.Infof("Started multistream to target. targetId=%s stream=%s url=%s", wildcardPlaybackID, targetRef.ID, selectorURL) | ||
info.mu.Unlock() | ||
glog.Infof("Started multistream to target. targetId=%s stream=%s url=%s", wildcardPlaybackID, targetRef.ID, pushURL) |
Check failure
Code scanning / CodeQL
Log entries created from user input
729de88
to
a8726e5
Compare
var videoSelector string | ||
// Not actually the source. But the highest quality. | ||
if targetRef.Profile == "source" { | ||
videoSelector = "maxbps" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just hit a bug with this in MistProcLivepeer where sometimes a transcoded rendition can be higher bitrate. @Thulinma is there a correct way for us to select just the source hre?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR as we chatted about this, this is not a behavior change in this PR, it was only copied from the previous code. We can merge this as is and follow up with a fix for this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MistProcLivepeer, since an update we rolled out during the holidays, can no longer select its own renditions as source for itself. That should handle this situation without any changes needed.
To answer the actual question: yes, sort-of. You can simply not specify any specific track selector (leave it default value), and set the default track selector to id_lth
to prioritize the lowest-ID track of the type, which for live streams is guaranteed to be the source (or one of the source tracks, at least, if there is more than one video track in the source already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at the context, I see this is actually about pushes rather than MistProcLivepeer itself. The same solution applies there, though: use no track selector and make sure the default selector sorting is set to id_lth
to make this work as intended.
(We could introduce a specific "non-modified tracks only" selector to make this easier to do ad-hoc, too! Would be a small change, only a couple lines of code.)
227d229
to
9d9b7fd
Compare
just in case we ever change the push url logic
This reverts commit 3006d46.
4f89955
to
699ed54
Compare
defer wg.Done() | ||
updated, err := mc.lapi.DeactivateMany(ids) | ||
|
||
glog.Infof("getStreamInfo: Fetching stream not found in memory. playbackID=%s", playbackID) |
Check failure
Code scanning / CodeQL
Log entries created from user input
// Assume setup was all successful | ||
multistreamStarted: true, | ||
} | ||
glog.Infof("getStreamInfo: Created info lazily for stream. playbackID=%s id=%s numPushes=%d", playbackID, stream.ID, len(pushes)) |
Check failure
Code scanning / CodeQL
Log entries created from user input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-LGTM before merge
This is to remove the assumption on mapic code that it runs forever alongside mist and
allow for it to be restarted.
For this, removed 2 main aspects of the code where mapic behaved in a stateful way: