873003 - Ignore duplicate video frames from NotifyQueuedTrackChanges()

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

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

In bug 861050, it was pointed out that NotifyQueuedTrackChanges was getting called for video tracks way more than every actual camera frame.

The getUserMedia() code will return the same Image pointer if there isn't a new frame from the camera, but apparently those cause a NotifyQueuedTrackChanged().

I did a quick measurement, and I was getting ~92 duplicate frames (and 30 non-dups) per second per pipeline/stream, and we were calling SendVideoFrame() for each.  I haven't verified if the GIPS code was dropping any of these; it very well might be dropping most of the duplicates due to bandwidth and internal framerate counters.

Comment 1

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

Attachment: ignore duplicate frames from NotifyQueuedTrackChanges()

Comment 2

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

Comment on attachment 750393 [details] [diff] [review]
ignore duplicate frames from NotifyQueuedTrackChanges()

I'll take roc or derf, whomever can get to it first :-)

Comment 3

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

Comment on attachment 750393 [details] [diff] [review]
ignore duplicate frames from NotifyQueuedTrackChanges()

Review of attachment 750393 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +364,5 @@
>      volatile bool active_;
>  
> +    // never dereference this; only to detect duplicate frames passed from
> +    // NotifyQueuedTrackChanges()
> +    void *last_img_;

This is dangerous. Since you're not keeping a ref, it's possible for the image pointed to by last_img_ to die, and for a new image to be allocated there, and for the pointer comparison to succeed when the images are in fact different.

You could use Image::GetSerial instead. Those are only 32-bit so maybe we should make them 64-bit for safety, but basically that seems like a good solution.

Comment 4

[Eric Rescorla (:ekr)]

I don't think this belongs in MediaPipeline. Rather, the MSG should suppress duplicates.

Comment 5

[Timothy B. Terriberry (:derf)]

Comment on attachment 750393 [details] [diff] [review]
ignore duplicate frames from NotifyQueuedTrackChanges()

Review of attachment 750393 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +364,5 @@
>      volatile bool active_;
>  
> +    // never dereference this; only to detect duplicate frames passed from
> +    // NotifyQueuedTrackChanges()
> +    void *last_img_;

Why not just acquire a ref? I realize VideoFrame doesn't allow that, but that's fixable. MediaEngineWebRTCVideoSource is already holding one anyway.

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 750393 [details] [diff] [review]
ignore duplicate frames from NotifyQueuedTrackChanges()

Review of attachment 750393 [details] [diff] [review]:
-----------------------------------------------------------------

See comment on bug. I think this is the wrong place for this change.

Comment 7

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

(In reply to Timothy B. Terriberry (:derf) from comment #5)
> Comment on attachment 750393 [details] [diff] [review]
> ignore duplicate frames from NotifyQueuedTrackChanges()
> 
> Review of attachment 750393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> @@ +364,5 @@
> >      volatile bool active_;
> >  
> > +    // never dereference this; only to detect duplicate frames passed from
> > +    // NotifyQueuedTrackChanges()
> > +    void *last_img_;
> 
> Why not just acquire a ref? I realize VideoFrame doesn't allow that, but
> that's fixable. MediaEngineWebRTCVideoSource is already holding one anyway.

Since we already have issues in B2G with limited camera buffers being available, I was trying to minimize the chance of holding one a buffer longer than needed (even a little longer), especially as we don't have need (in the pipeline) for any data in the image.

(In reply to Eric Rescorla (:ekr) from comment #4)
> I don't think this belongs in MediaPipeline. Rather, the MSG should suppress
> duplicates.

As far as MSG is concerned, the image is a duplicate but the meta-info isn't - we respond to NotifyPull(duration) with an image (new or not) and the requested duration to avoid the graph blocking while also avoiding the graph missing images (or getting them delayed more than the minimum possible to avoid blocking).  So for NotifyQueuedTrackChanges the updated duration is 'new' and should be passed on.

If the argument is "images should be pushed in and not have duration" - I agree, but that's a much bigger change and re-design.  If/when that's done, we could back this out.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Comment on attachment 750393 [details] [diff] [review]
> ignore duplicate frames from NotifyQueuedTrackChanges()
> 
> Review of attachment 750393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> @@ +364,5 @@
> >      volatile bool active_;
> >  
> > +    // never dereference this; only to detect duplicate frames passed from
> > +    // NotifyQueuedTrackChanges()
> > +    void *last_img_;
> 
> This is dangerous. Since you're not keeping a ref, it's possible for the
> image pointed to by last_img_ to die, and for a new image to be allocated
> there, and for the pointer comparison to succeed when the images are in fact
> different.

This can't happen for getUserMedia()-sourced streams (see comments), but in theory could happen for other streams, and if so might cause a skipped frame (not exactly dangerous, but wrong.)

> You could use Image::GetSerial instead. Those are only 32-bit so maybe we
> should make them 64-bit for safety, but basically that seems like a good
> solution.

32-bit is good enough for this use.

Comment 8

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #7)
> (In reply to Timothy B. Terriberry (:derf) from comment #5)
> > Comment on attachment 750393 [details] [diff] [review]
> > ignore duplicate frames from NotifyQueuedTrackChanges()
> > 
> > Review of attachment 750393 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> > @@ +364,5 @@
> > >      volatile bool active_;
> > >  
> > > +    // never dereference this; only to detect duplicate frames passed from
> > > +    // NotifyQueuedTrackChanges()
> > > +    void *last_img_;
> > 
> > Why not just acquire a ref? I realize VideoFrame doesn't allow that, but
> > that's fixable. MediaEngineWebRTCVideoSource is already holding one anyway.
> 
> Since we already have issues in B2G with limited camera buffers being
> available, I was trying to minimize the chance of holding one a buffer
> longer than needed (even a little longer), especially as we don't have need
> (in the pipeline) for any data in the image.
> 
> (In reply to Eric Rescorla (:ekr) from comment #4)
> > I don't think this belongs in MediaPipeline. Rather, the MSG should suppress
> > duplicates.
> 
> As far as MSG is concerned, the image is a duplicate but the meta-info isn't
> - we respond to NotifyPull(duration) with an image (new or not) and the
> requested duration to avoid the graph blocking while also avoiding the graph
> missing images (or getting them delayed more than the minimum possible to
> avoid blocking).  So for NotifyQueuedTrackChanges the updated duration is
> 'new' and should be passed on.
> 
> If the argument is "images should be pushed in and not have duration" - I
> agree, but that's a much bigger change and re-design.  If/when that's done,
> we could back this out.
>

How about instead we have a way to respond to NotifyPull with
"same as last time" and then MSG can do the suppression. 


> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> > Comment on attachment 750393 [details] [diff] [review]
> > ignore duplicate frames from NotifyQueuedTrackChanges()
> > 
> > Review of attachment 750393 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> > @@ +364,5 @@
> > >      volatile bool active_;
> > >  
> > > +    // never dereference this; only to detect duplicate frames passed from
> > > +    // NotifyQueuedTrackChanges()
> > > +    void *last_img_;
> > 
> > This is dangerous. Since you're not keeping a ref, it's possible for the
> > image pointed to by last_img_ to die, and for a new image to be allocated
> > there, and for the pointer comparison to succeed when the images are in fact
> > different.
> 
> This can't happen for getUserMedia()-sourced streams (see comments), but in
> theory could happen for other streams, and if so might cause a skipped frame
> (not exactly dangerous, but wrong.)
> 
> > You could use Image::GetSerial instead. Those are only 32-bit so maybe we
> > should make them 64-bit for safety, but basically that seems like a good
> > solution.
> 
> 32-bit is good enough for this use.

Comment 9

[Timothy B. Terriberry (:derf)]

(In reply to Eric Rescorla (:ekr) from comment #8)
> How about instead we have a way to respond to NotifyPull with
> "same as last time" and then MSG can do the suppression. 

Technically we do, since this listener ignores NULL frames, even if they have a non-zero intrinsic size. But I don't think that's the semantics they're supposed to have.

It also seems to me like if we're capturing at 30 fps, we should be able to just set the duration to 33 ms past the capture time at a minimum. That wouldn't solve the whole problem by itself, but it would eliminate a lot of thread churn.

Comment 10

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

(In reply to Eric Rescorla (:ekr) from comment #8)
> > (In reply to Eric Rescorla (:ekr) from comment #4)
> > > I don't think this belongs in MediaPipeline. Rather, the MSG should suppress
> > > duplicates.
> > 
> > As far as MSG is concerned, the image is a duplicate but the meta-info isn't
> > - we respond to NotifyPull(duration) with an image (new or not) and the
> > requested duration to avoid the graph blocking while also avoiding the graph
> > missing images (or getting them delayed more than the minimum possible to
> > avoid blocking).  So for NotifyQueuedTrackChanges the updated duration is
> > 'new' and should be passed on.
> > 
> > If the argument is "images should be pushed in and not have duration" - I
> > agree, but that's a much bigger change and re-design.  If/when that's done,
> > we could back this out.
> >
> 
> How about instead we have a way to respond to NotifyPull with
> "same as last time" and then MSG can do the suppression. 

We already do (by passing the same image with an updated duration).  The issue is that MSG needs to inform the consumers for the track of the change in duration, which it does in the same way (same Image, new duration).

Unless you change to a duration-less model (or add an "indefinite" duration value or track type), the call to NotifyQueuedTrackChanges is needed.

Comment 11

[Eric Rescorla (:ekr)]

> We already do (by passing the same image with an updated duration).  The
> issue is that MSG needs to inform the consumers for the track of the change
> in duration, which it does in the same way (same Image, new duration).
> 
> Unless you change to a duration-less model (or add an "indefinite" duration
> value or track type),

Which seems like exactly what we want for video, no?


> the call to NotifyQueuedTrackChanges is needed.

Well, it too could use the semantics of "this is a duplicate"

My point is that the consumer shouldn't have to keep track
of the old image in order to detect duplicates. Since until
we have video as indefinite time, that code will need to
be replicated everywhere.

Also, casting things to void * makes me sad.

Comment 12

[Eric Rescorla (:ekr)]

Related question:
We have almost exactly the same code in the pipeline for *incoming* video. Does
the rendering code detect that they are duplicates and suppress it? Or are we
just blitting the same image to the screen over and over?

Comment 13

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

(In reply to Timothy B. Terriberry (:derf) from comment #9)
> (In reply to Eric Rescorla (:ekr) from comment #8)
> > How about instead we have a way to respond to NotifyPull with
> > "same as last time" and then MSG can do the suppression. 
> 
> Technically we do, since this listener ignores NULL frames, even if they
> have a non-zero intrinsic size. But I don't think that's the semantics
> they're supposed to have.

Correct.

> It also seems to me like if we're capturing at 30 fps, we should be able to
> just set the duration to 33 ms past the capture time at a minimum. That
> wouldn't solve the whole problem by itself, but it would eliminate a lot of
> thread churn.

Well, that can cause problems, as it's known that "30fps" settings on cameras do not always produce even 33.3ms capture rates, even if they meet their 30fps average.  (Sigh)  The result might be unneeded dups and skips of frames, depending on a lot of timing issues (think about a sequence of 10ms/20ms/70ms; the 20ms frame would get skipped almost always and never even seen by the pipeline).

Would it work better than telling the encoder to encode 100+ times per second?  Sure!  But so does suppressing dups.  And right now (unfortunately) we don't have a hard link between track time and capture time, so there might be issues there too (especially if the stream were to stall).

Also, 30fps is the default but in some cases we might capture at >30fps or <30fps (25 may be somewhat common in europe), but of course we could set the duration to the nominal framerate.  I'm also not sure it will help thread churn much though, as the graph runs every 10ms normally.  All that said, it's an option, just one I'm concerned will leave some hard-to-diagnose quality issues I'd rather not sweep under the carpet.

We also could set some flag on the track that does "I don't care about duration changes", and move this logic into MSG.  That wouldn't actually help much, except avoid some callback overhead, since we'd still poll with NotifyPull.  And the MSG API would get more complex.

If we really want to reduce thread-switch/comparison/callback overhead, move to a duration-less model, where we can capture (if needed) at 10fps (or 60fps or 1fps) and push the frames into MSG with no NotifyPulls at all, no buffering of Images in gUM, etc.

I'd rather do the dup-suppression now, and if there's still a performance issue worth measuring, we can use a different solution.

Comment 14

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

(In reply to Eric Rescorla (:ekr) from comment #12)
> Related question:
> We have almost exactly the same code in the pipeline for *incoming* video.
> Does
> the rendering code detect that they are duplicates and suppress it? Or are we
> just blitting the same image to the screen over and over?

We only get DeliverFrame() callbacks from GIPS when there's a new frame from the other side.  We insert it into the MSG tracks in the same manner as gUM.  My understanding from roc was that media elements already dup-check; he was likely assuming we were too.  (I have not verified this, but I remember it coming up when I was redoing MSG to solve issues cause by the old insertion code).

Comment 15

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #14)
> (In reply to Eric Rescorla (:ekr) from comment #12)
> > Related question:
> > We have almost exactly the same code in the pipeline for *incoming* video.
> > Does
> > the rendering code detect that they are duplicates and suppress it? Or are we
> > just blitting the same image to the screen over and over?
> 
> We only get DeliverFrame() callbacks from GIPS when there's a new frame from
> the other side.  We insert it into the MSG tracks in the same manner as gUM.

Yes. It's this buffering stage that creates the problem.


> My understanding from roc was that media elements already dup-check; he was
> likely assuming we were too.  (I have not verified this, but I remember it
> coming up when I was redoing MSG to solve issues cause by the old insertion
> code).

That seems worth checking.

Comment 16

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

(In reply to Eric Rescorla (:ekr) from comment #11)
> > We already do (by passing the same image with an updated duration).  The
> > issue is that MSG needs to inform the consumers for the track of the change
> > in duration, which it does in the same way (same Image, new duration).
> > 
> > Unless you change to a duration-less model (or add an "indefinite" duration
> > value or track type),
> 
> Which seems like exactly what we want for video, no?

Yes, I (and Tim I think) have made the argument before (multiple times).  roc has indicated recently he'd be willing to reconsider this given a good usecase (which this might be).  But it's not a small change.

> > the call to NotifyQueuedTrackChanges is needed.
> 
> Well, it too could use the semantics of "this is a duplicate"

per above quote, it already does tell us it's a dup (same Image).

> My point is that the consumer shouldn't have to keep track
> of the old image in order to detect duplicates. Since until
> we have video as indefinite time, that code will need to
> be replicated everywhere.
> 
> Also, casting things to void * makes me sad.

I have an updated one that uses GetSerial() now that I know it exists.

Comment 17

[Timothy B. Terriberry (:derf)]

(In reply to Randell Jesup [:jesup] from comment #16)
> Yes, I (and Tim I think) have made the argument before (multiple times). 
> roc has indicated recently he'd be willing to reconsider this given a good
> usecase (which this might be).  But it's not a small change.

Well, only some consumers need to know the duration. Specifically, those that would write this data to a file (e.g., StreamRecorder), since most file formats need to know this when storing the frame. If we don't pass frame durations, they have to buffer until the next frame. If instead we pass in lots of little duplicate frames like we're doing now, they _still_ have to buffer until they receive a non-duplicate, except this is now non-obvious from the API, and as this patch shows, can have other bad effects.

So yeah, I think it's worth doing this change. Let's do it right instead of adding more hacks.

Comment 18

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

Attachment: ignore diplicate frames from NotifyQueuedTrackChanges()

This is the GetSerial() version of the original patch

Comment 19

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

(In reply to Randell Jesup [:jesup] from comment #14)
> We only get DeliverFrame() callbacks from GIPS when there's a new frame from
> the other side.  We insert it into the MSG tracks in the same manner as gUM.
> My understanding from roc was that media elements already dup-check; he was
> likely assuming we were too.

MSG dup-checks on its video rendering path. See MediaStreamGraphImpl::PlayVideo.

(In reply to Randell Jesup [:jesup] from comment #16)
> Yes, I (and Tim I think) have made the argument before (multiple times). 
> roc has indicated recently he'd be willing to reconsider this given a good
> usecase (which this might be).  But it's not a small change.

I definitely want to make this change. Sorry I wasn't clear about that. But it's probably weeks away unless someone else takes it. The GetSerial patch is very simple and I think we should take it.

Comment 20

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

Via wireshark, I verified we're sending ~30fps in a loopback call (pc_test.html).  I didn't find the bit that was deciding to throw away the other frames, though I did verify it runs through a bunch of optional image-processing code first before it makes any such decision, all of which *would* run on every frame if they were turned on.

We should verify how much difference this patch makes to performance on B2G as a guideline to how critical this issue is.  CJ, can you and your team get a first-order measurement of the CPU-use difference with and without this patch?  I'm especially interested in any changes that are large enough to affect framerate, say on the order of 3% CPU or more.  Thanks!

Comment 21

[Eric Rescorla (:ekr)]

Comment on attachment 750711 [details] [diff] [review]
ignore diplicate frames from NotifyQueuedTrackChanges()

Review of attachment 750711 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +789,5 @@
>    // We now need to send the video frame to the other side
>    layers::Image *img = chunk.mFrame.GetImage();
>    if (!img) {
>      // segment.AppendFrame() allows null images, which show up here as null
> +    last_img_ = -1;

Why do we need this? ISTM that there are three cases here:

1. last_img_ == -1 in which case this is a noop.
2. last_img_ != -1 and the next frame has serial != last_img_, in
   which case we would proceed to the loop below.
3. last_img_ != -1 and the next frame has serial == last_img_,
   in which case we should return now, but because you have set last_image_ = -1
   we now send a duplicate.

So, this only makes a difference in the third case, in which it seems to make
the situation worse.

What am I missing?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +329,1 @@
>          buffer_current_(0), samplenum_10ms_(0){}

Please either pack these all on one line or have one initializer per line, rather than being inconsistent.

Comment 22

[StevenLee[:slee]]

I did the profiling. Both peers are
* 176x144
* every 2 video frames drop one in WebrtcVideoConduit::SendVideoFrame
* video only

Before patching, the cpu usage is about 64~66%. After patching, the cpu usage is about 36~38%. And the video stream is smooth in both versions.

Comment 23

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

Attachment: ignore duplicate frames from NotifyQueuedTrackChanges()

Comment 24

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

Comment on attachment 755381 [details] [diff] [review]
ignore duplicate frames from NotifyQueuedTrackChanges()

nits addressed

Comment 25

[Eric Rescorla (:ekr)]

This doesn't want to make an interdiff... can you supply?

Comment 26

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

Sorry, didn't build it as an interdiff

All I did was remove the last_image_ = -1 and redo the init list to be one-per-line.  That's it.

Comment 27

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

Attachment: interdiffs

Comment 28

[Eric Rescorla (:ekr)]

Comment on attachment 755381 [details] [diff] [review]
ignore duplicate frames from NotifyQueuedTrackChanges()

Review of attachment 755381 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 29

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

https://hg.mozilla.org/projects/cypress/rev/c4912157c2d0

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4912157c2d0

Comment 31

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

Comment on attachment 755381 [details] [diff] [review]
ignore duplicate frames from NotifyQueuedTrackChanges()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Excessive CPU use (especially on mobile), some possible dropped frames

Testing completed (on m-c, etc.): On m-c.  Tested locally with wireshark

Risk to taking this patch (and alternatives if risky): Very low, very simple patch with no complex behavior

String or IDL/UUID changes made by this patch: none

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d34f5ea0327b