833824 - Need a way to add tracks to a MediaStream "now"

Status: RESOLVED WONTFIX

(Core :: Audio/Video: MediaStreamGraph, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Right now the MediaStream code does AdvanceKnownTracksTime() to the heat death of the universe on adding a track (to avoid ever blocking and make sure old data is pruned and dropped).  The downside of this is per MediaStreamGraph.h, you can't add Tracks once you've done that (at least from the text).

We have use cases where we need to add a track to a stream at an arbitrary point.  We don't care about any data "in the past", we just want to add it and if needed clip any old data for the stream. We'd like to call AddTrack() either with 0 aStart or some way to say "add as soon as possible" (negative aStart?)

While MSG.h says you can't call AddTrack with aStart less than the last AdvanceKnownTracksTime(), we currently do this in gUM:

stream->AddTrack(0,... 0, ...);
stream->AdvanceKnownTracksTime(STREAM_TIME_MAX);
stream->AddTrack(1,... 0, ...);
stream->AdvanceKnownTracksTime(STREAM_TIME_MAX);

Admittedly, a degenerate case, but AddTrack is definitely not enforcing the restriction.

We could go to production without dynamic track addition support (perhaps with a little pain), so blocking-.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This should be pretty easy to fix. Just call AdvanceKnownTracksTime in NotifyPull for the time interval requested.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Ok.

We considered this (on IRC); our concern with it was that it is happening async to AddTrack (and so if the stipulation in MSG.h is ever enforced, we'd randomly fail), or we'd need additional mutexes just to avoid that.  Also, AdvanceKnownTracksTime() currently takes at least two mutex grabs/releases, while not actually doing anything that is helping a realtime stream.  (I think it could be collapsed to one Mutex grab, but still.)

If the API stays as it is, we'll have to do as you suggest, and we'll have to stipulate that AddTrack() can use a start time "in the (recent) past" without failing, or we'll have to put the start time enough in the future to avoid any chance of triggering the (not-currently-checked) API requirement.  (Note: it's not really clear to me from MSG.h how aStart should be calculated for tracks that start after stream creation time, though I can guess.)

I'd prefer an API (or variant I can invoke with aStart == -1 or some such) where I could either ignore AdvanceKnownTracksTime() or call it once at the start, but then add tracks as needed to be played ASAP.  Ironically, I think the code will actually handle this with little or no modification.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randell Jesup [:jesup] from comment #2)
> We considered this (on IRC); our concern with it was that it is happening
> async to AddTrack (and so if the stipulation in MSG.h is ever enforced, we'd
> randomly fail), or we'd need additional mutexes just to avoid that.

Isn't the same code going to be responsible for calling AddTrack and AdvanceKnownTracksTime? Is the code that calls AddTrack going to be on a different thread from the one calling AdvanceKnownTracksTime?

> I'd prefer an API (or variant I can invoke with aStart == -1 or some such)
> where I could either ignore AdvanceKnownTracksTime() or call it once at the
> start, but then add tracks as needed to be played ASAP.  Ironically, I think
> the code will actually handle this with little or no modification.

AdvanceKnownTracksTime is there so that we can detect an underrun if the code feeding the SourceMediaStream is going to add a track for a specific start time but hasn't yet, and we need the data for that start time. I suppose we can get rid of it entirely if we instead enforce the invariant that callers must add new tracks for time T before adding data for existing tracks at time >= T. How does that sound?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> (In reply to Randell Jesup [:jesup] from comment #2)
> > We considered this (on IRC); our concern with it was that it is happening
> > async to AddTrack (and so if the stipulation in MSG.h is ever enforced, we'd
> > randomly fail), or we'd need additional mutexes just to avoid that.
> 
> Isn't the same code going to be responsible for calling AddTrack and
> AdvanceKnownTracksTime? Is the code that calls AddTrack going to be on a
> different thread from the one calling AdvanceKnownTracksTime?

No, you suggested calling AdvanceKnownTracksTime from NotifyPull aka MSG thread.  And some of our data is written to the track from a callback from a GIPS thread (video frames), or it's queued waiting for a NotifyPull.

> 
> > I'd prefer an API (or variant I can invoke with aStart == -1 or some such)
> > where I could either ignore AdvanceKnownTracksTime() or call it once at the
> > start, but then add tracks as needed to be played ASAP.  Ironically, I think
> > the code will actually handle this with little or no modification.
> 
> AdvanceKnownTracksTime is there so that we can detect an underrun if the
> code feeding the SourceMediaStream is going to add a track for a specific
> start time but hasn't yet, and we need the data for that start time. 

Sure, but that's not a case that exists for realtime flows.

> I suppose we can get rid of it entirely if we instead enforce the invariant
> that callers must add new tracks for time T before adding data for existing
> tracks at time >= T. How does that sound?

Same problem as above: We're not on the same thread typically as the data pulls/pushes, so we'd have to add mutexes to comply with the restriction safely, or clip a bunch ourselves to be reasonably sure the start time was in the future compared to sample insertion.

Realtime flows inherently have a different set of constraints from non-realtime (blocking) playback.

All we really need here is to add a track < current knowntracks time - and a) I don't believe the requirement in the comment is in any way enforced, and b) I'm totally fine (and in fact want) that if the track start time is before the current playback time it gets clipped.

Right now we're ignoring this (unenforced) restriction, though so far only for adding two tracks at the same time with an AdvanceKnownTracks(HEAT_DEATH) inbetween.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Paul - is this overtaken by events now? Or soon?

Comment 6

[Paul Adenot (:padenot)]

I think so, if all goes according to plans.

Comment 7

[Maire Reavy [:mreavy]]

Overtaken by events