To re-pro, 1. Go to https://shaka-player-demo.appspot.com/demo/#asset=https://storage.googleapis.com/wvtest/shaka-packager/historical/v2.2/misc/ClearPackaged/clear_tears.mpd;lang=en-US;build=uncompiled 2. select Load and Begin playback. 3. Tears begins and after a few seconds into playback responds with: Shaka Error MEDIA.VIDEO_ERROR (3,,) with spinner on screen The problem occurs when player was trying to adapt to a higher resolution. It is common to see H264 streams with base profile and main profile in the same AdaptationSet. The H264 stream with base profile will not have EditList while it is common for the H264 stream with main profile to contain an EditList. The same manifest works in other browsers. I don't see it being disallowed in the spec either.
<rdar://problem/45342208>
Here's the logging from a debug build of WebKit: SourceBufferPrivateAVFObjC::layerDidReceiveError(0x553ef7d00): layer(0x7f8d9f75cbb0), error(Error Domain=AVFoundationErrorDomain Code=-11800 "The operation could not be completed" UserInfo={AVErrorMediaSubTypeKey=( 1635148593 ), NSLocalizedDescription=The operation could not be completed, NSLocalizedFailureReason=An unknown error occurred (-8969), AVErrorMediaTypeKey=vide, AVErrorPresentationTimeStampKey=CMTime: {184832/15360 = 12.033}, NSUnderlyingError=0x7f8da0d28030 {Error Domain=NSOSStatusErrorDomain Code=-8969 "codecBadDataErr"}}) Looking for that timestamp in the enqueue log leads to: SourceBufferPrivateAVFObjC::enqueueSample(0x553ef7d00) - sample({PTS({184320/15360 = 12.000000}), OPTS({184320/15360 = 12.000000}), DTS({184320/15360 = 12.000000}), duration({512/15360 = 0.033333}), flags(1), presentationSize(318.133026x142.000000)}) SourceBufferPrivateAVFObjC::enqueueSample(0x553ef7d00) - sample({PTS({185344/15360 = 12.066667}), OPTS({185344/15360 = 12.066667}), DTS({184320/15360 = 12.000000}), duration({512/15360 = 0.033333}), flags(1), presentationSize(1917.759644x856.000000)}) SourceBufferPrivateAVFObjC::enqueueSample(0x553ef7d00) - sample({PTS({184832/15360 = 12.033333}), OPTS({184832/15360 = 12.033333}), DTS({184832/15360 = 12.033333}), duration({512/15360 = 0.033333}), flags(0), presentationSize(318.133026x142.000000)}) SourceBufferPrivateAVFObjC::enqueueSample(0x553ef7d00) - sample({PTS({186368/15360 = 12.133333}), OPTS({186368/15360 = 12.133333}), DTS({184832/15360 = 12.033333}), duration({512/15360 = 0.033333}), flags(0), presentationSize(1917.759644x856.000000)}) So somehow we are leaving in the decoding queue two sets of samples with the same DTS.
I don't think this has anything to do with edit lists, per se. But rather, there's a bug either in the spec or in our implementation that's particular to B-frames. The removal algorithm mandates the UA remove samples who overlap in presentation time, but in this case, the samples do not overlap in PTS at all. Rather, they overlap in decode time.
Actually, those samples should not be removed, because that would result in a "gap" in presentation order from 184320/15360 -> 185344/15360. One other solution would be to re-order the decodeQueue to enqueue samples out-of-DTS order, but that might cause problems in the decoder.
I think the best solution we can hope for is, when were are re-enqueuing samples after a removal, we simply fail to enqueue samples which indicate a size change, but are not sync-samples.
Okay, I've come around 180º from my previous opinion. I think that those frames whose decode times are after the replacement frame's decode times should be removed as well. I have a patch which implements and tests this.
Created attachment 352706 [details] Patch
Comment on attachment 352706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352706&action=review > Source/WebCore/Modules/mediasource/SampleMap.cpp:316 > + // endKey is not inclusive, so use lower_bound to exclude samples which start exactly at endKey. Nit: don't you mean "... so use upper_bound to ..." > LayoutTests/media/media-source/media-source-append-overlapping-dts.html:4 > + <title>mock-media-source</title> Nit: a more descriptive title would be nice.
Comment on attachment 352706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352706&action=review >> Source/WebCore/Modules/mediasource/SampleMap.cpp:316 >> + // endKey is not inclusive, so use lower_bound to exclude samples which start exactly at endKey. > > Nit: don't you mean "... so use upper_bound to ..." Nope, lower_bound. STL iterators are always inclusive-start, exclusive-end, so using lower_bound ensures that anything exactly matching endKey is excluded from the range. >> LayoutTests/media/media-source/media-source-append-overlapping-dts.html:4 >> + <title>mock-media-source</title> > > Nit: a more descriptive title would be nice. good point.
Created attachment 352741 [details] Patch for landing
Comment on attachment 352741 [details] Patch for landing Clearing flags on attachment: 352741 Committed r237271: <https://trac.webkit.org/changeset/237271>
All reviewed patches have been landed. Closing bug.
(In reply to KongQun Yang from comment #0) > It is common to see H264 streams with base profile and main profile in the > same AdaptationSet. The H264 stream with base profile will not have EditList > while it is common for the H264 stream with main profile to contain an > EditList. This comment scared me a bit, since edit list displace the timestamps of the movies and in general, you don't want adaption sets to be offsetted with each other. Later I checked the files and it's fine in this case: the edit list in the main profile AdaptationSet comes just from the fact that the main profile (unlike the baseline profile) uses B-frames, and coding B-frames in MP4 causes offsetting of frames which has to be compensated with an edit list.