New issue
Advanced search Search tips
Starred by 6 users

Issue metadata

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


Sign in to add a comment

Tracking bug for MSE PTS/DTS compliance work

Project Member Reported by wolenetz@chromium.org, May 4 2017

Issue description

Multiple sub-issues track different aspects of this cleanup. This issue tracks the whole bag of issues.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 6 2017

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

commit bf671ef01f848c38d2259f98b4fdc24c9666f5db
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Tue Jun 06 21:11:47 2017

Rename SBR::IsNextInSequence

We'll need an additional IsNextInPresentationSequence method as part of
PTS/DTS compliance. This change renames the existing method to
IsNextInDecodeSequence, adds comments, moves the implementation of the
method to align with declaration order in the header, and adds a
CHECK(!buffers_.empty()) precondition check.

BUG= 718641 
R=chcunningham@chromium.org

Change-Id: Idb44cb85a7821ef6d37a7e677c8e03729aa732bb
Reviewed-on: https://chromium-review.googlesource.com/526372
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477403}
[modify] https://crrev.com/bf671ef01f848c38d2259f98b4fdc24c9666f5db/media/filters/source_buffer_range.cc
[modify] https://crrev.com/bf671ef01f848c38d2259f98b4fdc24c9666f5db/media/filters/source_buffer_range.h

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 23 2017

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

commit c485d6c706fe27268948c43e71ef9f68e743bcf7
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Fri Jun 23 00:18:50 2017

MSE: Calculate SourceBufferRange range end PTS times

As part of PTS/DTS compliance, SourceBufferRange will need to
understand its presentation interval and understand whether or not a
subsequent keyframe is "continuous" with the end of that interval. This CL adds
tracking of the range's frame with highest PTS, adds a method (for test usage
only at the moment) to query whether or not a frame is continuous in PTS with
the end of the range, relocates IsNextInDecodeSequence to private scope, and
adds some better comments. It also adds some unit tests exercising the new
behavior.

This change does not switch over to using PTS for actually managing
buffered ranges, but significantly prepares for that to happen in later
CL(s).

BUG= 718641 

Change-Id: Iabd19071032d4508abbb9c62a5f7c897d334670b
Reviewed-on: https://chromium-review.googlesource.com/541586
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481736}
[modify] https://crrev.com/c485d6c706fe27268948c43e71ef9f68e743bcf7/media/filters/source_buffer_range.cc
[modify] https://crrev.com/c485d6c706fe27268948c43e71ef9f68e743bcf7/media/filters/source_buffer_range.h
[modify] https://crrev.com/c485d6c706fe27268948c43e71ef9f68e743bcf7/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/c485d6c706fe27268948c43e71ef9f68e743bcf7/media/filters/source_buffer_stream_unittest.cc

Blockedon: 354518
Labels: -M-60 M-61
Blockedon: 728477
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 8 2017

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

commit 495493bad91c1fb4dd30462bb04b6f79c96ecc88
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Sat Jul 08 01:33:04 2017

MSE: Detect and MEDIA_LOG one kind of problematic GOP structure

If a random access point doesn't have the earliest presentation time of
other frames that depend on it (eg, other frames in later decode time up
until the next random access point), the MSE spec was not designed to
support processing and buffering it well. With the change to managing
and reporting buffered ranges by PTS intervals instead of DTS intervals,
this could impact interop. This change detects this general case and
logs once per track to chrome://media-internals. Later changes might
include telemetry collection to assist removing or fixing support for
at least SAP Type 2 in the MSE ISOBMFF bytestream spec.

To verify the new log is emitted by the new test, this change also
upgrades FrameProcessorTest's |media_log_| to a StrictMock<MockMediaLog>
and includes new strict verification of logs emitted during
FrameProcessorTests.

See also related spec issue https://github.com/w3c/media-source/issues/187

BUG=739931, 718641 

Change-Id: I361177dee6a5c70edf17bdbde2f3ea643977e6ec
Reviewed-on: https://chromium-review.googlesource.com/563017
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485125}
[modify] https://crrev.com/495493bad91c1fb4dd30462bb04b6f79c96ecc88/media/base/test_helpers.h
[modify] https://crrev.com/495493bad91c1fb4dd30462bb04b6f79c96ecc88/media/filters/frame_processor.cc
[modify] https://crrev.com/495493bad91c1fb4dd30462bb04b6f79c96ecc88/media/filters/frame_processor_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 13 2017

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

commit f68e45d65cc00063ffc9cfb84e5b632bb8252104
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Thu Jul 13 23:00:40 2017

MSE: Report UseCounter and RAPPOR for two problematic MSE usages

This change adds UseCounter (for publicly visible % of Page Visits
statistics) and RAPPOR reporting to help us gauge the frequency of these
types of MSE API usages encountered in Chrome usage.  Such data may
assist subsequent work to clarify the spec, improve our implementation,
and potentially deprecate support.

"KeyframeTimeGreaterThanDependant" reporting: (Bug 739931)
If nonkeyframe's PTS precedes the PTS of the keyframe necessary to
decode it, this (in ISO-BMFF terminology) is not SAP Type 1 (though
other bytestreams might also encounter this GOP structure, too.) Our
buffering mechanism should gracefully handle this type of GOP, but the
MSE spec is unclear for how to precisely report resulting buffered
ranges and handle buffering/playback of overlaps of these types of
streams.

"MuxedSequenceMode" reporting: (Bug 737757)
If a multitrack SourceBuffer is used in 'sequence' AppendMode, the spec
leads to frequently surprising and undesirable results, usually due to
automatic timestampOffset updates based on one track after a
discontinuity are applied to all tracks.

At least bug 739931 may impact current PTS/DTS compliance work tracked
by  bug 718641 .

BUG=739931,737757, 718641 

Change-Id: I4fabb4ae0b389c5ce2eecb361d4b67c6d4874b04
Reviewed-on: https://chromium-review.googlesource.com/567558
Reviewed-by: Rick Byers <rbyers@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Alexei Svitkine (slow) <asvitkine@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486512}
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/base/test_helpers.h
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/blink/websourcebuffer_impl.cc
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/blink/websourcebuffer_impl.h
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/BUILD.gn
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/chunk_demuxer.cc
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/chunk_demuxer.h
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/chunk_demuxer_unittest.cc
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/frame_processor.cc
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/frame_processor.h
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/frame_processor_unittest.cc
[add] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/source_buffer_parse_warnings.h
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/source_buffer_state.cc
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/source_buffer_state.h
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/filters/source_buffer_state_unittest.cc
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/test/mock_media_source.cc
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/media/test/mock_media_source.h
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/third_party/WebKit/Source/modules/mediasource/SourceBuffer.h
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/third_party/WebKit/public/platform/WebSourceBufferClient.h
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/third_party/WebKit/public/platform/web_feature.mojom
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/f68e45d65cc00063ffc9cfb84e5b632bb8252104/tools/metrics/rappor/rappor.xml

Blockedon: -575824
Blocking: 575824
Labels: -M-61 M-62
Planning to begin experimentation in M62.
Blockedon: 759336
Blockedon: 758802
Blockedon: 761567
Blockedon: 754657
Labels: -M-62 M-63
@#11 s/62/63.
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 8 2017

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

commit c0f3bef80fffe5c7aa030801066f9f8fbe158508
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Fri Sep 08 20:38:14 2017

MSE: Adds feature flag for work to fix PTS/DTS compliance

The new feature flag is media::kMseBufferByPts, and is disabled by default
while working on the landing and experimenting with new buffering logic.

BUG= 718641 

Change-Id: I858287f206fcb702e75cf6d3d10bbfe1503246a9
Reviewed-on: https://chromium-review.googlesource.com/656564
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500649}
[modify] https://crrev.com/c0f3bef80fffe5c7aa030801066f9f8fbe158508/media/base/media_switches.cc
[modify] https://crrev.com/c0f3bef80fffe5c7aa030801066f9f8fbe158508/media/base/media_switches.h

Blockedon: 763528
Blockedon: -763528
Blockedon: 763518
Project Member

Comment 21 by bugdroid1@chromium.org, Sep 12 2017

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

commit f90e2676acbd70f4171fe24af56ac109f15ffbaf
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Tue Sep 12 20:27:29 2017

MSE: Begin SourceBufferStreamTests' testing of both old and new buffering APIs

Parameterizes all SourceBufferStreamTests by which version of the buffering API
is being tested; the test ctor forces on/off the media::kMseBufferByPts feature
accordingly.

Immediate follow-on work will be to enable the kNewByPts versions of these SBS
tests (possibly in incremental CLs) along with corresponding production impl,
while still also testing the 'legacy' buffering-by-DTS implementation.

BUG= 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: I40e29ac5775301ad69e305e9f0a521a5d5b3711a
Reviewed-on: https://chromium-review.googlesource.com/663879
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501381}
[modify] https://crrev.com/f90e2676acbd70f4171fe24af56ac109f15ffbaf/media/filters/source_buffer_stream_unittest.cc

Project Member

Comment 22 by bugdroid1@chromium.org, Sep 13 2017

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

commit 5741478d53d2f5bf44b4776dd9f3b36dc2e48530
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Sep 13 00:25:27 2017

MSE: Begin ChunkDemuxerTest'ing both old and new buffering APIs

Parameterizes all ChunkDemuxerTests by which version of the buffering
API is being tested; the test ctor forces on/off the
media::kMseBufferByPts feature accordingly.

BUG= 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: I28118da3a57b8f0156d8b399314333b2195d6e6b
Reviewed-on: https://chromium-review.googlesource.com/663785
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501477}
[modify] https://crrev.com/5741478d53d2f5bf44b4776dd9f3b36dc2e48530/media/filters/chunk_demuxer_unittest.cc

Project Member

Comment 23 by bugdroid1@chromium.org, Sep 13 2017

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

commit 17fcfc1b874e9d40e6e0aff06f6affcfeb7035fa
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Sep 13 00:33:36 2017

MSE: Begin FrameProcessorTest'ing both old and new buffering APIs

Previously, FrameProcessorTest had a boolean test parameter indicating
whether or not to test using sequence append mode. This change adds a
second dimension to the test parameter, which parameterizes the tests
further by which version of the buffering API is being tested. The test
ctor forces on/off the media::kMseBufferByPts feature accordingly.

It no longer uses test ctor initializer to construct the FrameProcessor,
since that must happen after adjusting enable/disable of the
kMseBufferByPts feature later in the ctor.

This change also captures the sequence mode boolean test parameter as a
protected member, including related test cleanup to use that member
instead of a local boolean or local GetParam() check.

Three previous TEST_F (that did not need an append mode variation) are
now TEST_P using the new FrameProcessorTestParams instantiations to keep
this change simpler.

BUG= 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: I9210900a29382bded23e3701b39af733864fceb2
Reviewed-on: https://chromium-review.googlesource.com/663765
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501483}
[modify] https://crrev.com/17fcfc1b874e9d40e6e0aff06f6affcfeb7035fa/media/filters/frame_processor_unittest.cc

Project Member

Comment 24 by bugdroid1@chromium.org, Sep 14 2017

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

commit 35daf1daab84886dbfb7154287d1835a32ed4a5c
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Thu Sep 14 17:31:45 2017

MSE: Begin PipelineIntegrationTest'ing both old and new buffering APIs

This change makes previous PipelineIntegrationTest cases which use the
MSE API become parameterized TEST_P MSEPipelineIntegrationTest cases,
with the parameter forcing kMseBufferByPts feature either on or off for
the test.

BasicMSEPlayback cases (which previously were already TEST_P, and
inherit PipelineIntegrationTest) are modified similarly to instantiate
versions of PlayToEnd for both on and off versions of kMseBufferByPts.

Some of the MSE cases used the "MAYBE_EME" macro to conditionally modify
the test name (to conditionally disable running the test). Once
converted to TEST_P, the MAYBE_EME macro was not expanding soon enough: the
runtime test names contained "MAYBE_EME(...)". To solve that, MAYBE_EME_TEST_P
macro is used to force expansion of MAYBE_EME(...) before test instantiation.

BUG= 718641 

Change-Id: Iee741f30ffb1b2286a54b9f59350f376468ee764
Reviewed-on: https://chromium-review.googlesource.com/666069
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501981}
[modify] https://crrev.com/35daf1daab84886dbfb7154287d1835a32ed4a5c/media/test/pipeline_integration_test.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Sep 20 2017

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

commit c32f337ca4c8362db63d3657bdf5b71d411652e5
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Sep 20 04:53:25 2017

MSE: Refactor SourceBufferRangeByDts out of SourceBufferRange

This CL prepares for having another kind of SourceBufferRange, one that
manages continuous buffered intervals by presentation interval.

BUG= 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: I54cac7552ff76b86826d29193b9cea4ad5afae62
Reviewed-on: https://chromium-review.googlesource.com/672007
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503054}
[modify] https://crrev.com/c32f337ca4c8362db63d3657bdf5b71d411652e5/media/filters/BUILD.gn
[modify] https://crrev.com/c32f337ca4c8362db63d3657bdf5b71d411652e5/media/filters/source_buffer_range.cc
[modify] https://crrev.com/c32f337ca4c8362db63d3657bdf5b71d411652e5/media/filters/source_buffer_range.h
[add] https://crrev.com/c32f337ca4c8362db63d3657bdf5b71d411652e5/media/filters/source_buffer_range_by_dts.cc
[add] https://crrev.com/c32f337ca4c8362db63d3657bdf5b71d411652e5/media/filters/source_buffer_range_by_dts.h
[modify] https://crrev.com/c32f337ca4c8362db63d3657bdf5b71d411652e5/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/c32f337ca4c8362db63d3657bdf5b71d411652e5/media/filters/source_buffer_stream.h
[modify] https://crrev.com/c32f337ca4c8362db63d3657bdf5b71d411652e5/media/filters/source_buffer_stream_unittest.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Sep 21 2017

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

commit 2d1b3e73b6e8b569953f830224545c8e1a7d6b94
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Thu Sep 21 03:44:27 2017

MSE: Template-ize SourceBufferStream with SourceBufferRange parameter

Makes SourceBufferStream's choice of SourceBufferRange subclass be a
template parameter "RangeClass". This will allow swap-in usage of a new
kind of SourceBufferRange (SourceBufferRangeByPts) in future CLs based
on which kind of SourceBufferStream is constructed, along with custom
SourceBufferStream logic for a subset of its methods where necessary
based on RangeClass choice.

For now, hardcodes construction in test and product to only test
SourceBufferRangeByDts. Includes preparation for refactoring
SourceBufferStreamTests to be TYPED_TESTs. Follow-up CLs will add
...ByPts variant.

BUG= 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: Ic159c632c58e58b33db2606676884a4ca3afecef
Reviewed-on: https://chromium-review.googlesource.com/673853
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503346}
[modify] https://crrev.com/2d1b3e73b6e8b569953f830224545c8e1a7d6b94/media/filters/chunk_demuxer.cc
[modify] https://crrev.com/2d1b3e73b6e8b569953f830224545c8e1a7d6b94/media/filters/chunk_demuxer.h
[modify] https://crrev.com/2d1b3e73b6e8b569953f830224545c8e1a7d6b94/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/2d1b3e73b6e8b569953f830224545c8e1a7d6b94/media/filters/source_buffer_stream.h
[modify] https://crrev.com/2d1b3e73b6e8b569953f830224545c8e1a7d6b94/media/filters/source_buffer_stream_unittest.cc

Project Member

Comment 27 by bugdroid1@chromium.org, Sep 21 2017

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

commit 7ce40f30c2aff299c9f67894702753c0ebd3695d
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Thu Sep 21 15:50:24 2017

MSE: Add SBRByPts and refactor SBSTest to work for both ByDts and ByPts

Adds SourceBufferRangeByPts implementation (which is currently a
copy of the ByDts implementation) to establish a working baseline of
SBSTests and SBRByPts prior to refactoring SBRByPts to actually do
buffering correctly ByPts.

Rather than using TYPED_TESTs (which would have required most test
case lines to be prefixed by "this->"), TestWithPararam/TEST_P, new
helper macros, and new conditional fixture logic are used to resolve
the correct SourceBufferStream<Range Type> being tested.

BUG= 718641 
TEST=183 existing LegacyByDts/SourceBufferRangeTests and 183 new NewByPts/SourceBufferRangeTests

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: Ib771f838ba7bbd133ed6d717d27c08c4fcfe4684
Reviewed-on: https://chromium-review.googlesource.com/674529
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503456}
[modify] https://crrev.com/7ce40f30c2aff299c9f67894702753c0ebd3695d/media/filters/BUILD.gn
[add] https://crrev.com/7ce40f30c2aff299c9f67894702753c0ebd3695d/media/filters/source_buffer_range_by_pts.cc
[add] https://crrev.com/7ce40f30c2aff299c9f67894702753c0ebd3695d/media/filters/source_buffer_range_by_pts.h
[modify] https://crrev.com/7ce40f30c2aff299c9f67894702753c0ebd3695d/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/7ce40f30c2aff299c9f67894702753c0ebd3695d/media/filters/source_buffer_stream_unittest.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Sep 21 2017

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

commit 8aa773b497e8ad75468e8fa0c0cd2ac6687a255a
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Thu Sep 21 16:53:24 2017

MSE: Vary ChunkDemuxer{Stream} of ByDts/ByPts based on media::kMseBufferByPts

Also adds kMseBufferByPts feature variation to SourceBufferStateTests
(since they construct ChunkDemuxerStreams, too).

Also includes some clean-up of SourceBufferStream enums (extracting
Status and Type from template-specialization to simplify usage.)

BUG= 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: I5531b8b9fbf10c5c6b2137fd436ba53a99526963
Reviewed-on: https://chromium-review.googlesource.com/675987
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503472}
[modify] https://crrev.com/8aa773b497e8ad75468e8fa0c0cd2ac6687a255a/media/filters/chunk_demuxer.cc
[modify] https://crrev.com/8aa773b497e8ad75468e8fa0c0cd2ac6687a255a/media/filters/chunk_demuxer.h
[modify] https://crrev.com/8aa773b497e8ad75468e8fa0c0cd2ac6687a255a/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/8aa773b497e8ad75468e8fa0c0cd2ac6687a255a/media/filters/source_buffer_state_unittest.cc
[modify] https://crrev.com/8aa773b497e8ad75468e8fa0c0cd2ac6687a255a/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/8aa773b497e8ad75468e8fa0c0cd2ac6687a255a/media/filters/source_buffer_stream.h
[modify] https://crrev.com/8aa773b497e8ad75468e8fa0c0cd2ac6687a255a/media/filters/source_buffer_stream_unittest.cc

Project Member

Comment 29 by bugdroid1@chromium.org, Sep 21 2017

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

commit 660c2bc51411c2744eab5378fdf26bee5a0b08ba
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Thu Sep 21 22:52:27 2017

MSE: Don't vary kMseBufferByPts in unit tests unnecessarily

Since the kMseBufferByPts feature enabled/disabled state is cached by
ChunkDemuxer's ctor (to coherently configure the MSE portion of all of
the buffering for that ChunkDemuxer and protect against feature status
possibly changing during the lifetime of the ChunkDemuxer), various unit
tests that test layers below (not including ChunkDemuxer) no longer need
to vary the feature state. They already vary how they configure the
system under test directly (like how ChunkDemuxer would).

This change removes unnecessary kMseBufferByPts toggling in these lower
layer tests:
  SourceBufferStateTests
  FrameProcessorTests
  SourceBufferStreamTests

Note: PipelineIntegrationTests and ChunkDemuxerTests must still do
feature toggling to retain coverage.

BUG= 718641 

Change-Id: I69e8e59d2b198e38e4f4a7f51ce37b70c862745e
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
Reviewed-on: https://chromium-review.googlesource.com/676536
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503593}
[modify] https://crrev.com/660c2bc51411c2744eab5378fdf26bee5a0b08ba/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/660c2bc51411c2744eab5378fdf26bee5a0b08ba/media/filters/source_buffer_state_unittest.cc
[modify] https://crrev.com/660c2bc51411c2744eab5378fdf26bee5a0b08ba/media/filters/source_buffer_stream_unittest.cc

Project Member

Comment 30 by bugdroid1@chromium.org, Sep 25 2017

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

commit 2b7cd80fef150134fbde862777253792904d5add
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Mon Sep 25 23:07:22 2017

MSE: Remove extraneous end_of_stream() check

SourceBufferStream should not be buffering EOS buffers internally. In
fact, any access of such a buffer's timestamp() method would DCHECK.
This change removes an extraneous condition for such buffers leftover
from long-removed cross-fade splicing code.

BUG= 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: Ie9a7bd9fb50195dd9f868462aae0879f5dedb6bc
Reviewed-on: https://chromium-review.googlesource.com/679734
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504201}
[modify] https://crrev.com/2b7cd80fef150134fbde862777253792904d5add/media/filters/source_buffer_range_by_dts.cc
[modify] https://crrev.com/2b7cd80fef150134fbde862777253792904d5add/media/filters/source_buffer_range_by_pts.cc

Project Member

Comment 31 by bugdroid1@chromium.org, Sep 25 2017

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

commit a0644c28a278b84511abf1e3dc42ac49e67e2857
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Mon Sep 25 23:26:59 2017

MSE: Remove unused SourceBufferRangeBy*::SeekAhead* methods

These 3 methods in each of SBRByPts and SBRByDts are not used by
anything, and are deleted by this change. I found these during
refactoring SBRByPts for PTS/DTS compliance, and split their deletion
into this CL to make compliance CL(s) smaller.

BUG= 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: I2a50a6e618e6d3bcc2a799bec398ebe752dba690
Reviewed-on: https://chromium-review.googlesource.com/683154
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504210}
[modify] https://crrev.com/a0644c28a278b84511abf1e3dc42ac49e67e2857/media/filters/source_buffer_range_by_dts.cc
[modify] https://crrev.com/a0644c28a278b84511abf1e3dc42ac49e67e2857/media/filters/source_buffer_range_by_dts.h
[modify] https://crrev.com/a0644c28a278b84511abf1e3dc42ac49e67e2857/media/filters/source_buffer_range_by_pts.cc
[modify] https://crrev.com/a0644c28a278b84511abf1e3dc42ac49e67e2857/media/filters/source_buffer_range_by_pts.h

Blocking: 769434
Blocking: -769434
Blockedon: 769434
https://chromium-review.googlesource.com/c/chromium/src/+/678443 is in review now. It adds real buffering-by-PTS behavior (when kMseBufferByPts feature is enabled).
Project Member

Comment 36 by bugdroid1@chromium.org, Sep 28 2017

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

commit b61fbbdfbb07406e7e293771f09aace9494bf582
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Thu Sep 28 22:02:53 2017

MSE: Fuzz using both old and new buffering APIs

This change sequentially attempts MSE fuzzing for both enabled and
disabled feature media::kMseBufferByPts for the same input.

BUG= 718641 

Change-Id: Id93677cde3c464b740465e4dfe9cfb202395813f
Reviewed-on: https://chromium-review.googlesource.com/664092
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505179}
[modify] https://crrev.com/b61fbbdfbb07406e7e293771f09aace9494bf582/media/test/pipeline_integration_fuzzertest.cc

Project Member

Comment 37 by bugdroid1@chromium.org, Sep 29 2017

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

commit e8819a83e72eb79e44ed0c6a7c73c4e53c04ed9b
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Fri Sep 29 01:33:46 2017

MSE: Log to chrome://media-internals the buffering API being used

Logs either "ChunkDemuxer: buffering by PTS" or ... "by DTS" to
chrome://media-internals on MSE demuxer construction. This assists in
manual verification and debugging.

Tested with/without --enable-features=MseBufferByPts in cmdline

BUG= 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: I418fb8e9d89bbfc98c343b2db6effc8a03201794
Reviewed-on: https://chromium-review.googlesource.com/689865
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505245}
[modify] https://crrev.com/e8819a83e72eb79e44ed0c6a7c73c4e53c04ed9b/media/base/test_helpers.h
[modify] https://crrev.com/e8819a83e72eb79e44ed0c6a7c73c4e53c04ed9b/media/filters/chunk_demuxer.cc
[modify] https://crrev.com/e8819a83e72eb79e44ed0c6a7c73c4e53c04ed9b/media/filters/chunk_demuxer_unittest.cc

Blocking: 630263
Project Member

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

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

commit 8efe7217bc8d16be91cc94b0de80f6a5bd5d704f
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Tue Oct 03 20:40:26 2017

MSE: Buffer media by PTS when kMseBufferByPts feature is enabled

Other than mechanical* DTS/PTS pieces, this CL contains the bulk of the PTS/DTS compliance work:
1) SBS<SBRByPts> and SBRByPts order buffers within GOP by DTS, between GOP by PTS.
   See especially SBRByPts::GetBufferIndexAt(...) and ::CanAppendBuffersToEnd(...)
2) SBS now has highest_timestamp_in_append_sequence_ and
   highest_buffered_end_time_in_append_sequence_, along with updated PrepareRangesForNextAppend(),
   to correctly *not* remove self-overlapping out-of-order buffers in a sequence of appends within
   the same coded frame group when buffering ByPts. Similarly, usage of and implementation of GC
   helper FreeBuffersAfterLastAppended() consults these to not unduly evict pieces of an
   out-of-order GOP that is before media_time.
3) SBS now has highest_output_buffer_timestamp_ to help prevent some undue stalling in cases
   like track buffer exhaustion with out-of-order track buffers when buffering ByPts.
4) FP signals SBS of coded frame group starts, including PTS information. SBS decides which to use
   based on ByDts vs ByPts. Also, FP handles re-signalling CFG start time if the start has moved
   earlier across tracks in a muxed SourceBuffer (similar to existing logic for DTS and muxed
   sequence mode). SAP-Type-2 is not explicitly rejected; related FP*OOO* unit tests demonstrate
   current buffering results ByDts vs ByPts for one simple SAP-Type-2 scenario.

Many related unit tests are updated. Notably, SBSTests already had out-of-order buffering
in many of its cases.

* Much of the mechanical stuff is to make SBRBy{Pts,Dts} use the intended time type internally,
  while letting SBS get away with using DTS still in many places where eventually it'll use PTS
  when SBRByDts gets dropped. See the set of new templated SBS::Range*(...) adapter methods.

BUG= 718641 , 769434 , 402502 , 398130 

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: Ib2abb816d240a6f74362bb83061ac4157dab210b
Reviewed-on: https://chromium-review.googlesource.com/678443
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506161}
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/chunk_demuxer.cc
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/chunk_demuxer.h
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/frame_processor.cc
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/frame_processor.h
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/source_buffer_range.cc
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/source_buffer_range.h
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/source_buffer_range_by_dts.cc
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/source_buffer_range_by_dts.h
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/source_buffer_range_by_pts.cc
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/source_buffer_range_by_pts.h
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/source_buffer_stream.h
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/filters/source_buffer_stream_unittest.cc
[modify] https://crrev.com/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f/media/test/pipeline_integration_test.cc

Blocking: 354518
Blockedon: -354518
Blockedon: -763518
Project Member

Comment 43 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)
#43 and especially #39 fix this issue.
Follow-up work:
1) experimenting with MseBufferByPts feature is tracked by (internal) launch bug 760264
2) eventually removing kLegacyByDts logic once MseBufferByPts ships always-on is tracked by bug 771349

For those who may want to try out buffering by PTS before it ships always on, use the --enable-features=MseBufferByPts cmdline on M63 builds containing #39 and #43 (e.g. October 4th or 5th canary should be the first such builds).

Comment 45 Deleted

Sign in to add a comment