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

Introducing frame based stream tracker. #1267

Merged
merged 9 commits into from
Dec 28, 2022
Merged

Conversation

boks1971
Copy link
Contributor

Design is to have different implementations. Currently supported are

  1. Packet based - the existing method
  2. Frame based - new method, will require more experimentation to iron out rough edges. The idea (or goal or hope :-) ) behind this is to have something that can work automatically for different source types without any configuration. It is not there yet, but hopefully this is a step along the way.

Also made a few other changes

  • Do not remove layer if not simulcast
  • Do not do FPS calculation when paused

Will drop notes inline to explain some of the bits.

BitrateReportInterval map[int32]time.Duration `yaml:"bitrate_report_interval,omitempty"`
ExemptedLayers []int32 `yaml:"exempted_layers,omitempty"`
PacketTracker map[int32]StreamTrackerPacketConfig `yaml:"packet_tracker,omitempty"`
FrameTracker map[int32]StreamTrackerFrameConfig `yaml:"frame_tracker,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworking this to contain all the stream tracker config for a specific track source. Top level config then has only Video and Screenshare.

@@ -431,7 +439,7 @@ func (b *Buffer) patchExtPacket(ep *ExtPacket, buf []byte) *ExtPacket {
}

func (b *Buffer) doFpsCalc(ep *ExtPacket) {
if b.frameRateCalculated || len(ep.Packet.Payload) == 0 {
if b.paused || b.frameRateCalculated || len(ep.Packet.Payload) == 0 {
Copy link
Contributor Author

@boks1971 boks1971 Dec 28, 2022

Choose a reason for hiding this comment

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

Passing along the paused state to Buffer. Main reason being, when camera is disabled on Firefox, looks like we cannot fully disable it. It sends 1 fps at layer 2 when disabled. That is causing the FPS calculator to calculate wrong frame rate for layer 2.


// ------------------------------------------------------------

type StreamTrackerImpl interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The impl that different stream tracker algorithms should implement.

Copy link
Member

Choose a reason for hiding this comment

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

nice!


type StreamTrackerParams struct {
StreamTrackerImpl StreamTrackerImpl
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the StreamTracker base struct that should be passed in an impl. This class does things like measuring bitrate, handling pause state, stream state etc.

I did another design where the actual implementation is the top level struct and those include this base struct so that callers could instantiate an implementation and be done with it. Now callers have to instantiate an impl and then put that in the params of base and instantiate base. Not too bad, but not ideal. But, the base embedded in top level became too complicate with locking and code sharing. So, went with this approach which turned out to be cleaner.

Logger logger.Logger
}

type StreamTrackerFrame struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new frame based stream tracker.

func (s *StreamTrackerFrame) Observe(hasMarker bool, ts uint32) StreamStatusChange {
if !s.initialized {
s.initialized = true
if hasMarker {
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 had this like

        // Start only on a key frame. This condition should hold true as this path is exercised under following conditions                                                 
        //   1. When track starts                                                                                                                                          
        //   2. When track resumes after an unmute.                                                                                                                        
        //   3. When a layer starts after dynacast pause.                                                                                                                  
        if isKeyFrame {                                                                                                                                                    
            s.initialized = true                                                                                                                                           
            if hasMarker {                                                                                                                                                 
                s.tsInitialized = true                                                                                                                                     
                s.oldestTS = ts                                                                                                                                            
                s.newestTS = ts                                                                                                                                            
                s.numFrames = 1                                                                                                                                            
            }                                                                                                                                                              
            return StreamStatusChangeActive                                                                                                                                
        }

But, it turned out to be not true. I thought browsers will use a key frame if the previous frame is too far back. But, they don't. If I disable camera, wait for 15 seconds and then re-start the camera, it sends a P frame with the old frame as reference. Makes sense to make a P frame if the old frame is in the encoder memory. So, removed that and just using re-start of traffic. This is effectively packet based, i. e. declaring active on first packet just like the packet based stream tracker works.

// STREAM-TRACKER-FRAME-TODO: might need to protect against one frame, long pause and then one or more frames, i. e. window getting stale.
// One possible option is to reset the fps measurement variables (tsInitialized, oldestTS, newestTS, numFrames, lowestFrameRate, evelInterval)
// and restart the lowest frame rate calulation process.
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noting a couple of bits with this marker. I could not reproduce in my testing, but something to think about.

@@ -0,0 +1,83 @@
package streamtracker
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No change in this algorithm, just split out into an impl.

exemptedLayersVideo []int32
exemptedLayersScreenshare []int32
trackerType config.StreamTrackerType
trackerConfig config.StreamTrackerConfig
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworking config made this simpler to just take Video or Screenshare based on track source and put it in this variable and use it.

s.trackers[layer] = tracker
s.lock.Unlock()

tracker.SetPaused(paused)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

paused also caused trouble with stream tracker. It was not passed in when a tracker was created. With FF, if the camera is disabled soon after start, layer 2 (the 1 FPS layer) starts up after pausing. That triggers OnTrack and a tracker is created. But, that tracker did not get the paused setting and proceeded to declare layer available seeing packets/frames from that layer.

@@ -414,7 +446,8 @@ func (s *StreamTrackerManager) removeAvailableLayer(layer int32) {

newLayers := make([]int32, 0, DefaultMaxLayerSpatial+1)
for _, l := range s.availableLayers {
if exempt || l != layer {
// do not remove layers for non-simulcast
if exempt || l != layer || len(s.trackInfo.Layers) < 2 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making this change to not remove layer for non-simulcast tracks.

s.trackerConfig = trackersConfig.Screenshare
case livekit.TrackSource_CAMERA:
s.trackerConfig = trackersConfig.Video
default:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using Video settings by default as some of the tests don't have TrackSource.

Copy link
Contributor

@cnderrauber cnderrauber 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!

Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

lg!


// ------------------------------------------------------------

type StreamTrackerImpl interface {
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@boks1971 boks1971 merged commit 2b031a5 into master Dec 28, 2022
@boks1971 boks1971 deleted the raja_stream_tracker branch December 28, 2022 07:30
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.

None yet

3 participants