New issue
Advanced search Search tips

Issue 850316 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Timestamp offset updating when generate_timestamps_flag is true, is different from spec when frames are dropped outside append window

Project Member Reported by wolenetz@chromium.org, Jun 7 2018

Issue description

The generate_timestamps_flag-conditioned timestampOffset update implementation assumes that, if frame processing itself doesn't update timestamp_offset, then the predicted frame end timestamp (in SourceBufferState::OnNewBuffers()) should be used as the timestampOffset increment.

This over-increments timestampOffset if the bufferQueue ended with one or more frames completely dropped by appendWindow trimming.

When generate_timestamps_flag is true:
* The spec processes each frame using current timestampOffset, with PTS and DTS set to 0 before applying the offset, and (only if successfully processed and not dropped) updates the timestampOffset per each frame.
* Chrome processes a *sequence of frames* using currenttimestampOffset, with PTS and DTS beginning at 0 and increasing by frame duration (each sequence emitted from the MPEGAudioStreamParserBase resets the timestamp helper, so the next sequence (or after a config change) begins again with time 0). Chrome then assumes that, either FrameProcessor detected a discontinuity (or handled entry into sequence mode from segments mode) and updated timestampOffset accordingly; and if no discontinuity, it assumes the last frame in the sequence made it through processing successfully. This last bit is faulty, because appendWindow{Start,End} trimming can drop complete frames, including that last frame.

Note, this issue was found during code inspection. I'm unaware of this issue causing problems for web authors, but it is a compliance issue not currently covered by any tests.
 
Project Member

Comment 1 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

Sign in to add a comment