New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 398141 link

Starred by 15 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 354518
issue 718641



Sign in to add a comment

SourceBufferStream should allow DTS to be negative as long as PTS is >= 0

Project Member Reported by acolwell@chromium.org, Jul 28 2014

Issue description

B-Frame content in MP4 files that don't use edit lists can have the presentation timestamp for random access points be greater than the decode timestamp. Edit lists are usually used to shift the presentation timestamps so they match the decode timestamps. If the edit lists aren't used and the web application tries to use timestampOffset to shift the presentation timestamps back to 0 the SourceBufferStream code will trigger an error because of the negative DTS timestamps. 

The test below exposes the situation I am referring to. The decode timestamps are negative on the first 2 packets to simulate a -60ms timestamp offset being applied to correct for the difference between DTS & PTS.

// Test simulates B-frame content where MP4 edit lists
// are not used to shift PTS, but a timestampOffset is used instead.
TEST_F(SourceBufferStreamTest, BFrames_ShiftedWithTimestampOffset) {
  Seek(0);
  NewSegmentAppend("0|-60K 120|-30 30|0 60|30 90|60");
  CheckExpectedRangesByTimestamp("{ [0,150) }");

  CheckExpectedBuffers("0|-60K 120|-30 30|0 60|30 90|60");
  CheckNoNextBuffer();
}

 
Labels: MSEscrubbedM45
 Issue 371947  has been merged into this issue.
Labels: MSEptsdtsCleanup
Labels: MSEscrubbed
Cc: wolenetz@chromium.org
 Issue 592465  has been merged into this issue.
This would have been fixed by now, except the allowance for negative DTS in the timeline would confound the API users because Chrome still reports DTS instead of PTS in various MSE API places (like buffered!).

The crbugs currently with label MSEptsdtsCleanup have our attention and we plan to commence fixing them (including this one) over the next few months.

For a little more history, the MSE spec, at one time, indicated we needed to issue this decode error when DTS < 0 at this point in the coded frame processing algorithm. But that piece was removed from the spec (see https://www.w3.org/Bugs/Public/show_bug.cgi?id=27487#c5). As part of fixing MSEptsdtsCleanup bugs, we'll fix this bug by updating Chrome's coded frame processing algorithm to *not* emit decode err for this case, while first fixing .buffered and other pieces of Chrome MSE that currently incorrectly report or act on DTS instead of PTS to use PTS correctly.
Cc: -wolenetz@chromium.org
Labels: MSE-compat M-53
Owner: wolenetz@chromium.org
Status: Assigned (was: Available)
Note that the FrameProcessor also has an obsolete (once PTS is correctly used by SBS) parse fail if the processed frame has a negative DTS. I'll add a TODO referencing this bug in that soon-to-be-obsolete FrameProcessor step in a CL shortly.
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b69a4ca4fb53925d723fe517a637b0d88736582

commit 8b69a4ca4fb53925d723fe517a637b0d88736582
Author: wolenetz <wolenetz@chromium.org>
Date: Wed Jun 15 18:36:29 2016

MSE: Update FrameProcessor comments w.r.t. current spec

The spec's coded frame processing algorithm steps have changed slightly.
This change updates the related code comments to link to the June 9,
2016 editor's draft, adds a TODO referencing  bug 398141 , and removes a
TODO referencing  bug 351166 .

BUG= 398141 , 351166 
R=chcunningham@chromium.org

Review-Url: https://codereview.chromium.org/2064073004
Cr-Commit-Position: refs/heads/master@{#399969}

[modify] https://crrev.com/8b69a4ca4fb53925d723fe517a637b0d88736582/media/filters/frame_processor.cc

Project Member

Comment 10 by sheriffbot@chromium.org, Jul 19 2016

Labels: -M-53 M-54 MovedFrom-53
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Blocking: 354518
matt, is this bug fixed by CL on #9?
@#12, no, SourceBufferStream still needs the PTS/DTS conflation fixed.
Labels: -M-54 M-56
Cc: wolenetz@chromium.org chcunningham@chromium.org
 Issue 679565  has been merged into this issue.
Blocking: 718641
Cc: sande...@chromium.org
Labels: -M-56 M-63
Status: Started (was: Assigned)
Fix is out for review @ https://chromium-review.googlesource.com/c/chromium/src/+/691431

The fix depends on first landing https://chromium-review.googlesource.com/c/chromium/src/+/678443

The fix is also conditional on MseBufferByPts feature being enabled.
Project Member

Comment 19 by bugdroid1@chromium.org, Oct 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b857b29abcd5bb55cdbd49755a8c291b37f65da

commit 4b857b29abcd5bb55cdbd49755a8c291b37f65da
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Tue Oct 03 22:04:23 2017

MSE: Allow negative DTS when MseBufferByPts feature is enabled

When compliantly processing frames and buffering by PTS interval,
negative DTS is no longer a trigger for parse error. This change
plumbs the buffering API (kLegacyByDts vs kNewByPts) being
used by the ChunkDemuxer into FrameProcessor and conditionally
allows negative DTS if buffering by PTS.

Includes related new FrameProcessorTests.

BUG= 398141 , 718641 

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I74c78d575ee26aa14c78e39b91ba7c6a303b3b87
Reviewed-on: https://chromium-review.googlesource.com/691431
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506198}
[modify] https://crrev.com/4b857b29abcd5bb55cdbd49755a8c291b37f65da/media/base/test_helpers.h
[modify] https://crrev.com/4b857b29abcd5bb55cdbd49755a8c291b37f65da/media/filters/chunk_demuxer.cc
[modify] https://crrev.com/4b857b29abcd5bb55cdbd49755a8c291b37f65da/media/filters/frame_processor.cc
[modify] https://crrev.com/4b857b29abcd5bb55cdbd49755a8c291b37f65da/media/filters/frame_processor.h
[modify] https://crrev.com/4b857b29abcd5bb55cdbd49755a8c291b37f65da/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/4b857b29abcd5bb55cdbd49755a8c291b37f65da/media/filters/source_buffer_state_unittest.cc

Status: Fixed (was: Started)
#19 fixes this (when MseBufferByPts feature is enabled).
Eventually removing the LegacyByDts code is tracked by bug 771349.
JFYI, I've just finished repro'ing and testing a bug report on Shaka Player [1], and I believe it was caused by this bug.  The issue does not reproduce in Chrome 63 beta with the MseBufferByPts flag.

[1] https://github.com/google/shaka-player/issues/1108
This fix seems to be necessary for another Shaka Player issue [1], but it is only available behind the MseBufferByPts flag.

Can the negative DTS tolerance part be brought out from behind the flag, or does it completely depend on the PTS buffering logic?

Put another way, when will regular end-users of video websites be able to benefit from this fix?

[1] https://github.com/google/shaka-player/issues/1194
Cc: joeyparrish@chromium.org
@#22, if the frame corresponding to the negative DTS needs to remain buffered and be available for decode and render later (along with any decode dependencies of that frame), then fixing that in the BufferingByDts (older) code is unlikely.

However, I think we could take an alternative approach (for a BufferingByDts patch) and just drop any frame with negative DTS (and drop its decode dependencies until the next keyframe). This would make the associated content with, or depending on, frame with DTS < 0 *not buffered, nor decoded or rendered* but would prevent MEDIA_ERR_DECODE from resulting. Similar logic is already in the coded frame processing algorithm for frames with PTS outside the append window; since BufferingByDts is technically out-of-spec, it seems such a patch would be preferable to decode error (especially since such content shouldn't produce decode error in (newer) BufferingByPts compliant logic).

joeyparrish@: What do you think? I think I could make a patch for this for M-66 in fairly short order.
Also @#22, "when will regular end-users of video websites be able to benefit from this fix?" --> I expect to begin experimentation with MseBufferByPts in M-66, too. I've been stabilizing the new code for a while, and it's getting close to being ready for experimentation now.
@#23, Yes, I think dropping would be preferable to decode failure.

@#24, great to hear!  Thanks for the update!

Comment 26 by s...@demoup.com, May 22 2018

I appreciate your fix. At least for me it works currently like a charm. When can we expect to get this fix also in the stable version? 
I would also like to express my appreciation for this fix, but I was curious as to when it would be available for the general public and not behind a flag.  I see this issue separately myself and would love to see this fix pushed to the public.  Otherwise, I will have to recommend the usage of Firefox instead to visitors due to video working there without issue.  
To be more specific, the issue is present with the following project code: https://github.com/enzanki-ars/eAVFD-test-01.  Adding --enable-features=MseBufferByPts has fixed the issue that I was seeing and brings it inline with the latest versions of Edge, Firefox, and Safari.

Sign in to add a comment