New issue
Advanced search Search tips

Issue 607372 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 605134



Sign in to add a comment

Comply with forcing appendMode to remain 'sequence' for MSE mp3 and adts stream parsers

Project Member Reported by wolenetz@chromium.org, Apr 28 2016

Issue description

<b>Version: <Kenneth, what is the frequency?></b>
<b>OS: <please tell me it's not XP></b>

What steps will reproduce the problem?
(1)
(2)
(3)

What is the expected output?

What do you see instead?


Please use labels and text to provide additional information.

 
Cc: chcunningham@chromium.org
Components: Internals>Media>Source
Labels: -Pri-3 MSE-compat OS-All Pri-2
Owner: wolenetz@chromium.org
Status: Assigned (was: Untriaged)
I hit enter too soon. So here's the detail:
Our mp3 and adts stream parser impls slightly predated the spec getting the generate-timestamps-flag behavior settled (and sequence appendMode).

We still need to force SourceBuffer.mode to be 'sequence' for these two stream types, and issue TypeError if their mode is attempted to be changed (per current spec).
Labels: MSEscrubbed
Labels: M-52
Project Member

Comment 4 by sheriffbot@chromium.org, Jun 1 2016

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

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 5 by sheriffbot@chromium.org, Jul 12 2016

Labels: -M-53 MovedFrom-53
This issue has been moved once and is lower than Pri-1. Removing the milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
See also https://codereview.chromium.org/2254093002/diff/60001/media/filters/media_source_state.cc#newcode807 where some related code needs fixing as part of fixing this bug.
Note, though  bug 623781  tracks the change to throw TypeError in many cases where previously we were throwing InvalidAccessError, one case is distinct and will need to be included in the fix for this bug:

When mode is forced to sequence (by generate-timestamps-flag): if app attempts to set mode to segments, throw TypeError.

Cc: sande...@chromium.org
Labels: M-69
Status: Started (was: Assigned)
Fix in in review now: https://chromium-review.googlesource.com/c/chromium/src/+/1086356
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 6 2018

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

commit a711806f720cc5a33d1ba0a07736b7dd2b36e751
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Jun 06 22:20:52 2018

MSE: Force sequence mode for bytestreams that generate timestamps

This change makes the web-visible SourceBuffer.mode attribute
compliantly set initially to 'sequence' and forced to remain 'sequence'
for byte streams which have their 'Generate Timestamps Flag' set in the
MSE Byte Stream Format Registry [1].

Prior to this change, for affected byte streams (audio/mpeg and
audio/aac), we would generate timestamps, but allow the
SourceBuffer.mode to vary among 'segments' and 'sequence'. We also would
not initialize the mode correctly to 'sequence' for those streams.

This should not be a breaking change, since generated timestamps mimic
'sequence' mode logic already, unless there is a pre-existing bug
causing discontinuities in those generated timestamps. But it does
compliantly set and restrict the SourceBuffer.mode attribute.

This change is useful also as a prerequisite for the 'changeType' work,
which will also need to conditionally set and constrain
SourceBuffer.mode based on the new MIME type's byte stream format.

[1] https://www.w3.org/TR/mse-byte-stream-format-registry/

BUG= 607372 
TEST=Related wpt layout FAIL expectations removed and tests pass locally
with proprietary codecs.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Ib5ac4e0430c75bd4c6fa00e76073dc83a9b987dc
Reviewed-on: https://chromium-review.googlesource.com/1086356
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565059}
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/base/mock_filters.h
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/base/stream_parser.h
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/blink/websourcebuffer_impl.cc
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/blink/websourcebuffer_impl.h
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/filters/chunk_demuxer.cc
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/filters/chunk_demuxer.h
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/filters/source_buffer_state.cc
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/filters/source_buffer_state.h
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/formats/mp2t/mp2t_stream_parser.cc
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/formats/mp2t/mp2t_stream_parser.h
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/formats/mp4/mp4_stream_parser.cc
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/formats/mp4/mp4_stream_parser.h
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/formats/mpeg/mpeg_audio_stream_parser_base.cc
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/formats/mpeg/mpeg_audio_stream_parser_base.h
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/formats/webm/webm_stream_parser.cc
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/media/formats/webm/webm_stream_parser.h
[delete] https://crrev.com/8de7421f9d286c8cbc1fc3f404e7158f541aca70/third_party/WebKit/LayoutTests/external/wpt/media-source/mediasource-addsourcebuffer-mode-expected.txt
[delete] https://crrev.com/8de7421f9d286c8cbc1fc3f404e7158f541aca70/third_party/WebKit/LayoutTests/external/wpt/media-source/mediasource-sourcebuffer-mode-timestamps-expected.txt
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/third_party/blink/public/platform/web_source_buffer.h
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/third_party/blink/renderer/modules/mediasource/media_source.cc
[modify] https://crrev.com/a711806f720cc5a33d1ba0a07736b7dd2b36e751/third_party/blink/renderer/modules/mediasource/source_buffer.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Jun 7 2018

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

commit e1119a8b0c01026a0b57ec3a0500ffb86141e803
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Thu Jun 07 18:18:50 2018

MSE: Deduplicate auto_update_timestamp_offset and generate_timestamps_flag

Since these two flags should map 1:1 to each other, this change removes
the former in favor of the latter, which is reachable from
SourceBufferState even before OnSourceInitDone has occurred.  It also
notes a pre-existing spec incompliance with newly filed crbug 850316.

BUG= 607372 ,850316

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I83d10db6b7c367615f5b968447dbdb415aee5cb7
Reviewed-on: https://chromium-review.googlesource.com/1089494
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565355}
[modify] https://crrev.com/e1119a8b0c01026a0b57ec3a0500ffb86141e803/media/base/stream_parser.cc
[modify] https://crrev.com/e1119a8b0c01026a0b57ec3a0500ffb86141e803/media/base/stream_parser.h
[modify] https://crrev.com/e1119a8b0c01026a0b57ec3a0500ffb86141e803/media/filters/frame_processor.cc
[modify] https://crrev.com/e1119a8b0c01026a0b57ec3a0500ffb86141e803/media/filters/source_buffer_state.cc
[modify] https://crrev.com/e1119a8b0c01026a0b57ec3a0500ffb86141e803/media/filters/source_buffer_state.h
[modify] https://crrev.com/e1119a8b0c01026a0b57ec3a0500ffb86141e803/media/formats/common/stream_parser_test_base.cc
[modify] https://crrev.com/e1119a8b0c01026a0b57ec3a0500ffb86141e803/media/formats/mp2t/mp2t_stream_parser_unittest.cc
[modify] https://crrev.com/e1119a8b0c01026a0b57ec3a0500ffb86141e803/media/formats/mp4/mp4_stream_parser_unittest.cc
[modify] https://crrev.com/e1119a8b0c01026a0b57ec3a0500ffb86141e803/media/formats/mpeg/mpeg_audio_stream_parser_base.cc

Status: Fixed (was: Started)
#11 fixed this, #12 cleaned up some leftover duplication.
Blocking: 605134

Sign in to add a comment