[Webkit-unassigned] [Bug 194747] New: [MSE] SourceBuffer sample time increment vs. last frame duration check is broken

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=194747

            Bug ID: 194747
           Summary: [MSE] SourceBuffer sample time increment vs. last
                    frame duration check is broken
           Product: WebKit
           Version: Safari Technology Preview
          Hardware: Unspecified
                OS: All
            Status: NEW
          Severity: Blocker
          Priority: P2
         Component: Media
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: up at nanocosmos.de

Technical Severity 

This issue is breaking MSE playback for totally MSE compliant streams, 
in our case fMP4/H.264/AAC, but not limited to this container or codecs. 
Effects are playback not starting at all or significantly delayed and  
massive amount of frame/sample drop mid stream forced by a 
wrong a MSE compliance check.  

Business Severity 

If the issue will make it into Safari 12.2, 
MSE playback will be broken in Safari for our cdn,  
business leading customers of ours and 
other MSE applications and users. 
We would like to offer assistance and developer time 
to prevent this. 

Code Location and Context 

Location of the issue is SourceBuffer::sourceBufferPrivateDidReceiveSample 
in WebCore/Modules/mediasource/SourceBuffer.cpp, 
in the second part of this conditional statement related to trackBuffer.greatestDecodeDuration. 
The problem is how trackBuffer.greatestDecodeDuration is set/calculated. 
It is mainly a logical issue. Please find more in the details section. 

// 1.6 ↳ If last decode timestamp for track buffer is set and decode timestamp is less than last
// decode timestamp:
// OR
// ↳ If last decode timestamp for track buffer is set and the difference between decode timestamp and
// last decode timestamp is greater than 2 times last frame duration:
if (trackBuffer.lastDecodeTimestamp.isValid() && (decodeTimestamp < trackBuffer.lastDecodeTimestamp
   || (trackBuffer.greatestDecodeDuration.isValid() && abs(decodeTimestamp - trackBuffer.lastDecodeTimestamp) > (trackBuffer.greatestDecodeDuration * 2)))) 

Bug history 

We have first discovered the issue in Safari Technology preview 75. 
We have been able to verify that it exists already in STP 72, 
and that it did NOT exist in STP 61. Further it does not exist in 
the current Safari 12.1 release. 

Checking out WebKit itself we have been able to recreate the issue 
in WebKit nightly 2019 Feb. 14th. 

We have checked in between [MEDIA] commits and found this one: 
https://trac.webkit.org/changeset/236314/webkit/

According to the description the trackBuffer.greatestDecodeDuration check 
has been hidden behind a smaller bug related to the assignment. 
By fixing this the major issue with trackBuffer.greatestDecodeDuration 
became active. 

Details 

Here is how we understand the timestamp increment vs last frame duration check 
in the MSE spec and how it is done in the current implementation. 

Example frame/sample sequence that should not fail (but currently does): 

Frame A) dts:0, duration 10 
Frame B) dts:10, duration 90 
Frame C) dts:100, duration 10 
Timestamp increase and previous frame duration are perfectly aligned. 

Example frame/sample sequence that should fail: 

Frame A) dts:0, duration 10 
Frame B) dts:10, duration 10 
Frame C) dts:100, duration 10 
Discontinuity due to mismatch from B to C. 

What should be compared is (C.dts - B.dts) vs. B.duration .

Current impl. is comparing (C.dts - B.dts) vs. (B.dts - A.dts) . 
(involving a maximum value storage but that does not matter here) 

Instead of comparing current timestamp increment vs. last frame duration 
it's comparing current timestamp increment vs. previous timestamp increment, 
which makes a huge difference, because the target here is not to check 
how constant the frame rate is, but to detect discontinuities. 
Not sure about other containers, but in fMP4 the timestamp and duration 
of a frame are stored separately and explicitely. 

Implications seem to be even stranger for audio in fMP4, 
where one sample can hold multiple audio frames. 
In this case the current check will apply constraints to 
how many audio frames one sample contains and how it 
compares to the number of audio frames in the previous sample, 
which is obviously not what this check should be about. 

Proposal for a 'simple' fix 

Logically the solution would be to compare against trackBuffer.lastFrameDuration 
if valid instead of trackBuffer.greatestDecodeDuration. 

Technically both approaches could be combined to keep changes at a minimum, 
but no sample should be rejected if trackBuffer.lastFrameDuration is not valid. 
It must be ensured that, if valid, trackBuffer.lastFrameDuration holds the duration 
of the last frame/sample, not of any previous ones. 

Thanks in advance!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190216/d1d3713c/attachment.html>