Bug 174505 - [MediaStream] Limit the number of remote video samples queued
Summary: [MediaStream] Limit the number of remote video samples queued
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-14 09:26 PDT by Eric Carlson
Modified: 2017-07-14 11:35 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch. (1.88 KB, patch)
2017-07-14 09:40 PDT, Eric Carlson
youennf: review+
Details | Formatted Diff | Diff
Patch for landing. (1.96 KB, patch)
2017-07-14 11:12 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2017-07-14 09:26:13 PDT
MediaPlayerPrivateMediaStreamAVFObjC::removeOldSamplesFromPendingQueue releases queued video samples that are earlier than the current stream time. This works for samples from a local media stream track, but frames from a remote track are tagged for "immediate display" and don't have valid time stamps so there is no such thing as an "old" sample.
Comment 1 Eric Carlson 2017-07-14 09:28:57 PDT
<rdar://problem/33223015>
Comment 2 Eric Carlson 2017-07-14 09:40:36 PDT
Created attachment 315435 [details]
Proposed patch.
Comment 3 youenn fablet 2017-07-14 10:23:01 PDT
Comment on attachment 315435 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=315435&action=review

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:263
> +    if (!queue.first()->decodeTime().isValid()) {

I applied the patch locally to check whether  decodeTime is invalid in the case of RealtimeIncomingVideoSource.
It is valid but is negative. At least on my Mac, we are not going through that loop.
We will probably go through the next while loop and empty the queue there.
Is it different on iOS?
Comment 4 Eric Carlson 2017-07-14 10:50:45 PDT
(In reply to youenn fablet from comment #3)
> Comment on attachment 315435 [details]
> Proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315435&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:263
> > +    if (!queue.first()->decodeTime().isValid()) {
> 
> I applied the patch locally to check whether  decodeTime is invalid in the
> case of RealtimeIncomingVideoSource.
> It is valid but is negative. At least on my Mac, we are not going through
> that loop.
> We will probably go through the next while loop and empty the queue there.
> Is it different on iOS?

While the raw PTS time is negative in the CMSampleBuffer:

(lldb) po (CMSampleBuffer)0x00007f88b20ca290
CMSampleBuffer 0x7f88b20ca290 retainCount: 1 allocator: 0x7fff8383df40
  ...
	numSamples = 1
	sampleTimingArray[1] = {
		{PTS = {-1/1 = -1.000}, DTS = {-1/1 = -1.000}, duration = {INVALID}},
	}
	sampleAttachmentsArray[0] = {
		sample 0:
			DisplayImmediately = true
	}
	imageBuffer = 0x7f88b5c2f140

"decodeTime" is invalid:

(lldb) p mediaSample.decodeTime()
(WTF::MediaTime) $11 = { Invalid } {
   = (m_timeValue = 0, m_timeValueAsDouble = 0)
  m_timeScale = 0
  m_timeFlags = '\0'
}
(lldb) p mediaSample.decodeTime().isValid()
(bool) $12 = false

But we should probably also check explicitly for negative times as well.
Comment 5 Eric Carlson 2017-07-14 11:12:39 PDT
Created attachment 315457 [details]
Patch for landing.
Comment 6 WebKit Commit Bot 2017-07-14 11:35:03 PDT
Comment on attachment 315457 [details]
Patch for landing.

Clearing flags on attachment: 315457

Committed r219513: <http://trac.webkit.org/changeset/219513>