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

Issue metadata

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


Sign in to add a comment
link

Issue 229412: Loosen media segments starting with a keyframe restriction

Reported by acolwell@chromium.org, Apr 9 2013 Project Member

Issue description

The current version of the MSE spec no longer requires that media segments start with a keyframe. The Chrome implementation needs to be updated to skip all non-keyframes before the first keyframe in a media segment.
 

Comment 1 by acolwell@chromium.org, Jun 13 2013

Blocking: chromium:239506

Comment 2 by t...@ubnt.com, Oct 14 2014

This is actually rather crucial for me. I have a web app that really needs to have low latency streams, and this is blocking it.

Anybody with any knowledge on this that can chime in?

It seems like Chrome is just arbitrarily complaining about media segments not starting with a keyframe, and that it doesn't really need them to?

https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/source_buffer_stream.cc&q=%22begin%20with%20a%20keyframe%22&sq=package:chromium&type=cs&l=191

Comment 3 by derek.bu...@gmail.com, Oct 15 2014

> The current version of the MSE spec no longer requires that media segments start with a keyframe. The Chrome implementation needs to be updated to skip all non-keyframes before the first keyframe in a media segment.

This seems wrong. It shouldn't skip the frames if it can be decoded properly (i.e. if it references IDR frames in previous segments that have been decoded.) For example, IE11 does this correctly.

Comment 4 by acolwell@chromium.org, Oct 15 2014

> This seems wrong. It shouldn't skip the frames if it can be decoded properly (i.e. if it references IDR frames in previous segments that have been decoded.) For example, IE11 does this correctly.

You are misunderstanding my intent. This bug is meant to cover dropping only the initial non-keyframes in the first media segment in a "coded frame group" since those frames can't be decoded. This is not intended to drop frames if a followup media segment starts with non-keyframes since we would have the appropriate keyframe from the segment appended earlier.

The current behavior is simply based on old legacy requirements from past versions of the MSE spec. This bug is just a placeholder to remind us to update that code to conform to the current spec text.

Comment 5 by derek.bu...@gmail.com, Oct 15 2014

> You are misunderstanding my intent. This bug is meant to cover dropping only the initial non-keyframes in the first media segment in a "coded frame group" since those frames can't be decoded. This is not intended to drop frames if a followup media segment starts with non-keyframes since we would have the appropriate keyframe from the segment appended earlier.

OK. Well Chromium is currently broken for followup segments which start with non-keyframes (it spits out an error about missing a keyframe to start a segment with).

Should I file a new bug then>

Comment 6 by acolwell@chromium.org, Oct 15 2014

>OK. Well Chromium is currently broken for followup segments which start with non-keyframes (it spits out an error about missing a keyframe to start a segment with).

>Should I file a new bug then>

I know that this is the current behavior. That is why this bug exists. Please don't file a new bug. This bug will be updated when this issue is fixed.

Comment 7 by derek.bu...@gmail.com, Oct 15 2014

Er, first you say "This bug is meant to cover dropping only the initial non-keyframes in the first media segment" but then you contradict yourself and say "I know that this is the current behavior. That is why this bug exists." in reference to the behavior in followup segments. That is a *direct* contradiction of yourself. It is unclear what the intent of this bug is now.

Comment 8 by t...@ubnt.com, Oct 15 2014

The issue is that Chromium complains if media segments do not begin with a keyframe. 

A side effect of removing this requirement from Chromium is that if the first segment does not begin with a keyframe, you would need to ignore all frames until you hit a keyframe.

It's still just one issue.

Comment 9 by derek.bu...@gmail.com, Oct 15 2014

OK.

You were unclear with your wording originally.

Comment 10 by acolwell@chromium.org, Oct 15 2014

I'm sorry this is confusing. In the code, these 2 behaviors are linked.

This bug is intended to change the existing behavior to the following.
1. Drop all non-keyframes at the beginning of a "coded frame group". 
2. Do not signal an error if a media segment does not begin with a keyframe. 

Typically #1 only involves the first media segment, but we may get a segment that only contains non-keyframes and we need to handle that. Item #2 will essentially fall out of #1, but care needs to be taken to make sure code doesn't remain that assumes media segment start and the first packet being a keyframe are equivalent. For quite a long time the start of a media segment and the first packet being a keyframe were considered equivalent and the code just needs to be checked very carefully to make sure that this assumption no longer exists in the code. There were good reasons for this assumption in the past, but the MSE spec has been updated to no longer require this. 

This may seem like a trivial change to you, but it is not. We just need to be very careful when updating the code so that we don't break large customers like YouTube and Netflix.

Comment 11 by derek.bu...@gmail.com, Oct 15 2014

Thanks for the clarification! I lacked the knowledge of how they were related in the codebase. Sorry if I was a tad confrontational.

(Side note: We're probably fairly big (Vimeo) in the process of working on DASH. :P)

Comment 12 by acolwell@chromium.org, Oct 15 2014

I am happy to see Vimeo starting to use MSE. If you happen to be using MP4Box to generate your DASH content, you can always use the --frag-rap to workaround this situation until this bug gets fixed. This works well for VOD content, but doesn't really help taka@'s live use case.

Comment 13 by t...@ubnt.com, Oct 15 2014

Perhaps I should detail my scenario to make sure it will in fact be solved by fixing this issue.

I have an h.264/aac stream being sent to a media server (interleaved).

Client opens a websocket connection to the media server, requesting a stream.

Media server repackages the stream as a never-ending fragmented MP4. Initially responds with an MOOV atom.

Whenever the media server encounters an IDR, it creates a MOOF/MDAT pair and sends it to the client.

On each MOOF/MDAT pair received over websockets, we call mediaSource.appendBytes();

Chromium treats each MOOF/MDAT pair as a new segment, which means that we currently need an IDR at the start of each MOOF/MDAT pair. 

This means we have an enforced latency of 1 IDR as we can't generate a MOOF atom without having the full MDAT that goes along with it. It also means that if a user requests the stream at a time just after an IDR occured, we have to wait almost 2 seconds before we send an MOOF/MDAT pair.

Ideally I could send whatever I want in a MOOF/MDAT pair without the keyframe requirement (i.e. not only should it not have to start with a keyframe, it shouldn't have to contain a keyframe at any point).

This would mean that I could send 200ms or even 100ms chunks instead of 1s chunks (increasing IDR frequency is not a viable solution as we also save these to disk).

Performance wise this is way better too, because I can have up to 16 streams on screen at once, and it is currently forcing Chromium to append large chunks at a time for each stream, if these chunks occur around the same time for each camera, then it's especially bad.

Comment 14 by shir...@gmail.com, Oct 15 2014

I'm facing an identical issue as the last poster. The MOOF/MDAT atoms should work with even only one frame. If the frame enqueued for decoding has references to previously missing IDRs (or any frame dependency), than that current frame should be simply ignored (black screen), while the audio should start immediately. When the first IDR comes in, all following frames should be fine. This would enable chrome to do real live streaming.

Someone pointed me to a portion of the Chrome's code and I have clearly saw a simple IF statement. Could we just remove it?

For people concerned with back compatibility: removing that IF should not be a problem, because it will enlarge the supported use cases. It will not add new constraints.

The link to the code:
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/source_buffer_stream.cc&l=191

Comment 15 by t...@gigafied.com, Dec 20 2014

Any updates on this?

Comment 16 by wolenetz@chromium.org, Jan 26 2015

Cc: wolenetz@chromium.org chada...@gmail.com
 Issue 452212  has been merged into this issue.

Comment 17 by wolenetz@chromium.org, Jan 29 2015

 Issue 452858  has been merged into this issue.

Comment 18 by wolenetz@chromium.org, Mar 21 2015

Cc: -wolenetz@chromium.org chcunningham@chromium.org
Owner: wolenetz@chromium.org
Status: Assigned
Following up on #10:
One further case that must be considered during implementing the fix is to handle correctly:

Append: {MediaSegmentA_with_keyframe.......}
Remove: {range including all MediaSegmentA.}
Append:                                    {MediaSegmentB_without_keyframe}{MediaSegmentC_with_keyframe}

If, between these two MediaSegments' Append, a Remove() occurred that removed the keyframe, the FrameProcessor is unaware of the need for a random access point, so the SourceBufferStream (which performed to internals of the Remove) must remember to drop the non-keyframe B (without error) and to appropriately move forward the range start time to keyframe C.

There may be other complex cases, but at the moment, this is the only one of which I am aware.

Comment 19 by wolenetz@chromium.org, Apr 9 2015

Labels: -Pri-2 Pri-1 M-44

Comment 20 by wolenetz@chromium.org, Apr 20 2015

Status: Started

Comment 21 by wolenetz@chromium.org, Apr 20 2015

I've started work (in-progress CL is currently at https://codereview.chromium.org/1089873006/).

Comment 22 by wolenetz@chromium.org, Apr 29 2015

Labels: MSE-bug-scrubbed-M44

Comment 23 by wolenetz@chromium.org, Apr 29 2015

Labels: -MSE-bug-scrubbed-M44 MSEscrubbedM44

Comment 24 by pennymac@google.com, May 21 2015

Labels: -M-44 MovedFrom-44 M-45
[AUTO] Moving all non essential bugs to the next Milestone.  (This decision is based on the labels attached to your ticket.)


Ref: https://sites.google.com/a/chromium.org/dev/developers/ticket-milestone-punting-1

Comment 25 by ddorwin@chromium.org, Jul 17 2015

Blocking: chromium:459546

Comment 26 by bugdroid1@chromium.org, Jul 21 2015

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

commit 7a7df715279034ae4e8ceffe9e1f74280212c031
Author: wolenetz <wolenetz@chromium.org>
Date: Tue Jul 21 00:04:04 2015

MSE: Provide link in media-internals to known  bug 229412  until it is fixed

Until  bug 229412  is fixed, this change at least provides a better
indication to developers when the bug is hit.

BUG= 229412 
R=ddorwin@chromium.org
TEST=manual

Review URL: https://codereview.chromium.org/1248453003

Cr-Commit-Position: refs/heads/master@{#339569}

[modify] http://crrev.com/7a7df715279034ae4e8ceffe9e1f74280212c031/media/filters/source_buffer_stream.cc

Comment 27 by wolenetz@chromium.org, Jul 21 2015

Labels: Merge-Request-45
Requesting merge of just #26 (the log improvement) to M45. This will help developers understand when this bug is hit, until this bug is fixed and that error log entry is removed.

Comment 28 by wolenetz@chromium.org, Jul 22 2015

Cc: amineer@chromium.org
+amineer@: Friendly ping regarding merge request in #27.

Comment 29 by amineer@chromium.org, Jul 22 2015

Labels: -Merge-Request-45 Merge-Approved-45

Comment 30 by bugdroid1@chromium.org, Jul 22 2015

Project Member
Labels: -Merge-Approved-45 merge-merged-2454
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/79c997d008c26d20d9a8f69950fad2c1d46a5c9e

commit 79c997d008c26d20d9a8f69950fad2c1d46a5c9e
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Jul 22 21:52:10 2015

To M45: MSE: Provide link in media-internals to known  bug 229412  until it is fixed

Until  bug 229412  is fixed, this change at least provides a better
indication to developers when the bug is hit.

Review URL: https://codereview.chromium.org/1248453003

Cr-Commit-Position: refs/heads/master@{#339569}
(cherry picked from commit 7a7df715279034ae4e8ceffe9e1f74280212c031)

Since LogCB deprecation is not merged to M45, the cherry-pick conflicted
and is resolved to use the older log_cb_ mechanism instead of
media_log_.

BUG= 229412 
TBR=ddorwin@chromium.org

Review URL: https://codereview.chromium.org/1249773006 .

Cr-Commit-Position: refs/branch-heads/2454@{#98}
Cr-Branched-From: 12bfc3360892ec53cd00fc239a47e5298beb063b-refs/heads/master@{#338390}

[modify] http://crrev.com/79c997d008c26d20d9a8f69950fad2c1d46a5c9e/media/filters/source_buffer_stream.cc

Comment 31 by bugdroid1@chromium.org, Jul 22 2015

Project Member
The following revision refers to this bug:
  https://chrome-internal.googlesource.com/bling/chromium.git/+/79c997d008c26d20d9a8f69950fad2c1d46a5c9e

commit 79c997d008c26d20d9a8f69950fad2c1d46a5c9e
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Jul 22 21:52:10 2015

Comment 32 by tri...@gmail.com, Jul 26 2015

Is there a workaround for this? Perhaps starting every segment with a stale IDR just to satisfy the IF statement?

Comment 33 by chcunningham@chromium.org, Aug 19 2015

Cc: sande...@chromium.org
 Issue 518854  has been merged into this issue.

Comment 34 by wolenetz@chromium.org, Aug 19 2015

@#32, unless the stale IDR had updated timestamps and duration, such a workaround risks triggering the "coded frame discontinuity" logic in the coded frame processor, which could lead to non-keyframes being dropped after the period of discontinuity following the stale IDR up to the point of the next keyframe. It would trade decode error for dropped frames, IIUC. The dropped frames could then also cause a gap in the buffered media timeline, requiring seek to prevent playback stall, etc.

Comment 35 by wolenetz@chromium.org, Aug 27 2015

Internal b/22779655 also hits this issue.

Comment 36 by galb...@gmail.com, Sep 10 2015

Hi, any estimation regarding a fix for this? I see that some work was done yesterday, but I'm not sure what's the overall status.

Comment 37 by wolenetz@chromium.org, Sep 10 2015

With the rebasing work I did yesterday to the first part of the fix, I did some further analysis as I paged this work-in-progress back in. Unless I'm mistaken, the coordination of frame evictions (GC or SourceBuffer.Remove()) with CFP for the purposes of aligning to some better specified logic (needing MSE spec update) as well as affording more interop for cases involving eviction in muxed SourceBuffers, is all secondary (and seems like would be separate issue) to separating the media segment vs coded frame group signalling. tl;dr: I think this signalling fix will make M47 barring any significant regression that my analysis hasn't discovered.

Comment 38 by t...@ubnt.com, Sep 10 2015

This issue is almost a year old :( would be great to get Chrome compliant to the latest spec.

Comment 39 by galb...@gmail.com, Sep 11 2015

Thanks. Really hoping to see it in M47.

Comment 40 by wolenetz@chromium.org, Sep 17 2015

Further detail on #37 w.r.t. #18: For segments appendMode, along with updates to coded frame processor to signal new coded frame group only when it has detected discontinuity, to handle the case shown in #18, SourceBufferStream will need to understand when the last-appended-GOP has been removed and to drop non-keyframes coming from CFP until next keyframe. Ideally, SBS would signal CFP that there's been a discontinuity and that (at least) the track corresponding to SBS needs a random access point. This latter route would allow the frame processor to more correctly align the next keyframe in 'sequence' append mode with the previous (now removed) group end time. Since this latter route involves the still-behind-flag 'sequence' append mode, I think it can be done in a subsequent CL so that the segments-mode keyframe relaxation might be mergable or at least a more granular CL.

Comment 41 by wolenetz@chromium.org, Sep 17 2015

Blockedon: chromium:533133

Comment 42 by wolenetz@chromium.org, Sep 17 2015

Comment 44 by a.ne...@atlas.cz, Oct 9 2015

Ok, just came across this issue. I am not able to feed H624 P frames as stand-alone segments to the media pipeline using MSE. Other browsers seem to accept this ok. I see that the status of this issue is "started". Is there an estimated date for this fix to appear in a Canary build?

Thanks

Alex

Comment 45 by cyrilcon...@gmail.com, Oct 9 2015

In case there is a need for more feedback to have this issue progress, I'd like to report my problems.

I'm working on MP4Box.js, a library to manipulate MP4 files in the browser. One the feature that people use is the on-the-fly fragmentation of non-fragmented MP4 for consumption using MSE. 

Currently MP4Box.js creates one fragment per frame, partly because it's easy to generate, partly because of latency issues as reported in this thread already. So not all fragments start with a RAP. So, the current code also marks all frames (including non RAP) as RAP (sync samples). This works around the current limitation of having segments start with RAP. However that creates artefacts when seeking. If I try to mark frames properly. It works well in FF but not in Chrome. I really would like this bug fixed to have the same behavior in FF and Chrome, with correct seeking.

Comment 46 by galb...@gmail.com, Oct 9 2015

@#45, Sounds like a nice workaround for live content. I'm gonna have to try this next week.
I just hope it won't have the opposite effect regarding the issue progress :)

Comment 47 by tri...@gmail.com, Oct 9 2015

@#45 awesome! are you able to share a code sample for what you have? specifically what might actually work in firefox? thank you!

Comment 48 by a.ne...@atlas.cz, Oct 23 2015

Ok, tried this, the aforementioned workaround for live content really works (thanks to CyrilCon...) ! If MP fragments containing H264 P frames pretend they are I frames (by manipulating the sample flags in the MP4 fragment correspondingly) Chrome decodes the stream correctly! This seems to indicate that the fix should not be that hard ;), but must involve loosening the condition of requiring a sync sample at the beginning of each MP4 fragment.

Comment 49 by wolenetz@chromium.org, Nov 24 2015

Labels: MSEscrubbed

Comment 50 by wolenetz@chromium.org, Jan 5 2016

Prerequisite MSE spec clarification is out for review today (https://github.com/w3c/media-source/pull/43).

Comment 51 by wolenetz@chromium.org, Jan 5 2016

Blockedon: chromium:542375

Comment 52 by bugdroid1@chromium.org, Jan 20 2016

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

commit 8d425ee590b8902b18a095c865e23470d6a176d7
Author: wolenetz <wolenetz@chromium.org>
Date: Wed Jan 20 02:20:26 2016

MSE: Log a warning if muxed AV media segment has no A or has no V block

To simplify debugging multiple discontinuity detection edge cases related
to the upcoming relaxation of the "media segments must begin with keyframe"
requirement in Chrome, this change introduces chrome://media-internals
debug log messages when either the audio or video track has no coded
frames within a media segment (for a muxed A/V SourceBuffer).

A previously redundant and now complicating call to the end of segment
callback in the WebM stream parser's Flush() is removed, and the comment
for interface StreamParser::Flush() is corrected to not refer to "Seek".

BUG= 229412 , 542375 
R=chcunningham@chromium.org
TEST=ChunkDemuxerTests

Review URL: https://codereview.chromium.org/1564983003

Cr-Commit-Position: refs/heads/master@{#370281}

[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/base/stream_parser.h
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/filters/chunk_demuxer_unittest.cc
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/filters/media_source_state.cc
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/filters/media_source_state.h
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/formats/mp2t/mp2t_stream_parser.cc
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/formats/mp2t/mp2t_stream_parser.h
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/formats/mp4/mp4_stream_parser.cc
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/formats/mp4/mp4_stream_parser.h
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/formats/mpeg/mpeg_audio_stream_parser_base.cc
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/formats/mpeg/mpeg_audio_stream_parser_base.h
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/formats/webm/webm_stream_parser.cc
[modify] http://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7/media/formats/webm/webm_stream_parser.h

Comment 53 by wolenetz@chromium.org, Jan 20 2016

I have a fix ready to land soon (https://codereview.chromium.org/1091293005). For available repros among mail threads and duplicates of this bug, I've tested this patch and it seems to work.

Comment 54 by wolenetz@chromium.org, Jan 22 2016

Blocking: chromium:580613

Comment 55 by wolenetz@chromium.org, Jan 22 2016

Blocking: chromium:580621

Comment 56 by bugdroid1@chromium.org, Jan 22 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3d152e528e97024860822d3e75109c0c597bf4de

commit 3d152e528e97024860822d3e75109c0c597bf4de
Author: wolenetz <wolenetz@chromium.org>
Date: Fri Jan 22 20:10:10 2016

MSE: Relax the 'media segment must begin with keyframe' requirement

The current version of the MSE spec no longer requires that media segments
start with a keyframe. This change reworks the MSE implementation
so that it now allows tracks within media segments to not begin
with a keyframe.

To accomplish this requirement's relaxation, this CL:
1) Separates media segment signaling from coded frame group signaling:
   MSE stream parsers need to report whether or not they are currently
   in the middle of parsing a media segment, such as a WebM cluster, to
   comply with spec disallowance of certain operations like changing
   SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT.
   However, SourceBufferStream only needs to be signaled when a new coded
   frame group is beginning, for example when buffers are appended
   following a discontinuity.
2) Since the FrameProcessor now can transitively signal SourceBufferStreams
   when a new coded frame group is beginning (which might span multiple
   media segments), the SourceBufferStream can rely upon the fact
   that the next buffer appended must be a keyframe if it's been told
   that a new coded frame group is starting. A CHECK() is included.
3) New ChunkDemuxerTests.
4) Various test cleanup to conform to the distinct "new coded frame group"
   and "new media segment" signaling separation, as well as removal
   of decode error verification when a media segment doesn't begin
   with a keyframe.

Note that the FrameProcessor already drops non-keyframes prior to the
first keyframe processed for a track following a discontinuity,
and the SourceBufferStream already drops non-keyframes prior to the
first keyframe processed for a track following a range removal that
removed the last appended position in the current coded frame group.

BUG= 229412 , 459546 
R=chcunningham@chromium.org
TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests,
FrameProcessorTests. Also locally tested:  bug 459546 's
original report no longer fails. A sample webm VP9 file
from b/26524063 no longer fails. GPM webm vp9/opus test no
longer fails.  Bug 518854 's original report's "flags_all.mp4"
no longer fails.

Review URL: https://codereview.chromium.org/1091293005

Cr-Commit-Position: refs/heads/master@{#371018}

[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/chunk_demuxer.cc
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/chunk_demuxer.h
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/chunk_demuxer_unittest.cc
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/frame_processor.cc
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/frame_processor.h
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/frame_processor_unittest.cc
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/media_source_state.cc
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/media_source_state.h
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/source_buffer_stream.cc
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/source_buffer_stream.h
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/filters/source_buffer_stream_unittest.cc
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/formats/mp2t/mp2t_stream_parser.cc
[modify] http://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de/media/formats/mp4/mp4_stream_parser.cc

Comment 57 by wolenetz@chromium.org, Jan 22 2016

Status: Fixed
The fix has landed in ToT.

I'll request merge to M49 of the following two CLs that combine to fix this issue once they've both made it to dev/canary channel release.

(#52): https://crrev.com/8d425ee590b8902b18a095c865e23470d6a176d7
(#56): https://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de

Comment 58 by wolenetz@chromium.org, Jan 25 2016

Status: Started
It looks like this caused a regression: see  bug 581125  for details. I'll either revert and reland with a fix for 581125 or just land a fix for 581125 soon.

Comment 59 by wolenetz@chromium.org, Jan 25 2016

Blockedon: chromium:581125

Comment 60 by wolenetz@chromium.org, Jan 27 2016

Blockedon: chromium:581458
Another regression: see  bug 581458  for details. Revert of https://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de is in CQ.

Both causes of regressions tracked in  bug 581458  and  581125  need to be fixed before relanding keyframe relaxation.

Comment 61 by bugdroid1@chromium.org, Jan 27 2016

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

commit 4d3c457f2301754ca4321c77c8ff13e69dec8acc
Author: wolenetz <wolenetz@chromium.org>
Date: Wed Jan 27 01:54:27 2016

Revert of MSE: Relax the 'media segment must begin with keyframe' requirement (patchset #13 id:240001 of https://codereview.chromium.org/1091293005/ )

Reason for revert:
Two regressions:
BUG= 581125 , 581458 

The latter causes NFLX playback stall. Reverting to fix canary, even if the latter's eventual fix might be external (e.g. NFLX web app).

To allow revert to currently failing code format rules:
NOPRESUBMIT=true

Original issue's description:
> MSE: Relax the 'media segment must begin with keyframe' requirement
>
> The current version of the MSE spec no longer requires that media segments
> start with a keyframe. This change reworks the MSE implementation
> so that it now allows tracks within media segments to not begin
> with a keyframe.
>
> To accomplish this requirement's relaxation, this CL:
> 1) Separates media segment signaling from coded frame group signaling:
>    MSE stream parsers need to report whether or not they are currently
>    in the middle of parsing a media segment, such as a WebM cluster, to
>    comply with spec disallowance of certain operations like changing
>    SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT.
>    However, SourceBufferStream only needs to be signaled when a new coded
>    frame group is beginning, for example when buffers are appended
>    following a discontinuity.
> 2) Since the FrameProcessor now can transitively signal SourceBufferStreams
>    when a new coded frame group is beginning (which might span multiple
>    media segments), the SourceBufferStream can rely upon the fact
>    that the next buffer appended must be a keyframe if it's been told
>    that a new coded frame group is starting. A CHECK() is included.
> 3) New ChunkDemuxerTests.
> 4) Various test cleanup to conform to the distinct "new coded frame group"
>    and "new media segment" signaling separation, as well as removal
>    of decode error verification when a media segment doesn't begin
>    with a keyframe.
>
> Note that the FrameProcessor already drops non-keyframes prior to the
> first keyframe processed for a track following a discontinuity,
> and the SourceBufferStream already drops non-keyframes prior to the
> first keyframe processed for a track following a range removal that
> removed the last appended position in the current coded frame group.
>
> BUG= 229412 , 459546 
> R=chcunningham@chromium.org
> TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests,
> FrameProcessorTests. Also locally tested:  bug 459546 's
> original report no longer fails. A sample webm VP9 file
> from b/26524063 no longer fails. GPM webm vp9/opus test no
> longer fails.  Bug 518854 's original report's "flags_all.mp4"
> no longer fails.
>
> Committed: https://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de
> Cr-Commit-Position: refs/heads/master@{#371018}

TBR=chcunningham@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 229412 , 459546 

Review URL: https://codereview.chromium.org/1637213002

Cr-Commit-Position: refs/heads/master@{#371686}

[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/chunk_demuxer.cc
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/chunk_demuxer.h
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/chunk_demuxer_unittest.cc
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/frame_processor.cc
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/frame_processor.h
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/frame_processor_unittest.cc
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/media_source_state.cc
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/media_source_state.h
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/source_buffer_stream.cc
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/source_buffer_stream.h
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/filters/source_buffer_stream_unittest.cc
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/formats/mp2t/mp2t_stream_parser.cc
[modify] http://crrev.com/4d3c457f2301754ca4321c77c8ff13e69dec8acc/media/formats/mp4/mp4_stream_parser.cc

Comment 62 by wolenetz@chromium.org, Feb 5 2016

Reland of fix for this bug, including fix for previous attempt's regressions ( bug 581125  and  bug 581458 ) is being prepared at https://codereview.chromium.org/1670033002

Comment 63 by wolenetz@chromium.org, Feb 5 2016

@ #45, #46, #47, #48: That solution (marking non-RAP as RAP) is problematic for some decoders. Specifically, on Mac OSX, in M48, we issue decode error for a non-IDR NAL unit type frame fed to a newly configured decoder because otherwise the Mac decoder would leak uninitialized memory to the decode output of an initial non-keyframe. See  bug 583638 .

While my fix for keyframe relaxation (this bug) should land in M50, that still leaves M48 and M49 issuing decode errors for those malformed streams on Macs that try to workaround this lack of relaxation. There's still a minor chance that the fix might get merged to M49, too, but that leaves M48 issuing decode error for these malformed streams on Macs.

Comment 64 by wolenetz@chromium.org, Feb 5 2016

sandersd@ is beginning work on a potentially mergeable-to-M48 fix for  bug 583638  which identifies when the first frame(s) ever fed to the mac decoder is(are) incorrectly marked as a keyframe, and just doesn't feed it(them) to the decoder. This would allow the workaround in #45-#48 for  bug 229412  to regress less, but eventually the plan would be to deprecate sandersd@'s Mac decoder fix once this keyframe relaxation has landed.

So heads-up: please don't expect the workaround in #45-48 to work on Chrome (M48) on Mac at the moment; it may work soon, but not for long after this keyframe relaxation is fixed.

Comment 65 by bugdroid1@chromium.org, Feb 6 2016

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

commit 8a2f5658c6301691342e302ea618c61cad5d220c
Author: sandersd <sandersd@chromium.org>
Date: Sat Feb 06 00:23:25 2016

Drop frames until first IDR in VTVDA.

This allows us to decode streams that incorrectly mark every frame
as a keyframe; we will drop frames until the first IDR and configure the
decoder then.

When this happens we log an error; it will show up in chrome://gpu (and
stderr).

This does mean that we are delaying decode failure (possibly indefinitely)
in that case.

BUG= 229412 , 583638 , 583698 , 516039 

Review URL: https://codereview.chromium.org/1679503002

Cr-Commit-Position: refs/heads/master@{#373964}

[modify] http://crrev.com/8a2f5658c6301691342e302ea618c61cad5d220c/content/common/gpu/media/vt_video_decode_accelerator_mac.cc
[modify] http://crrev.com/8a2f5658c6301691342e302ea618c61cad5d220c/content/common/gpu/media/vt_video_decode_accelerator_mac.h

Comment 66 by wolenetz@chromium.org, Feb 6 2016

#65 attempts to fix the initial decode error on incorrectly marked-in-container-as-keyframe non-IDR frames on Mac. It may likely require a follow-up CL to similarly drop non-IDR frames prior to the first IDR frame following a seek (VTVDA::Reset()) on Mac.

Comment 67 by wolenetz@chromium.org, Feb 10 2016

Blockedon: chromium:426575

Comment 68 by karishma...@gmail.com, Feb 15 2016

Is this issue fixed? Especially thecase where followup segments don't start with a keyframe? Chrome is the only browser erroring out

Comment 69 by dalecur...@chromium.org, Feb 17 2016

 Issue 586946  has been merged into this issue.

Comment 70 by bugdroid1@chromium.org, Feb 17 2016

Project Member
Labels: merge-merged-2623
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75bd1db3ad32a3e3439a5811ff461581d4a037e3

commit 75bd1db3ad32a3e3439a5811ff461581d4a037e3
Author: Dan Sanders <sandersd@chromium.org>
Date: Wed Feb 17 18:45:19 2016

Drop frames until first IDR in VTVDA.

This allows us to decode streams that incorrectly mark every frame
as a keyframe; we will drop frames until the first IDR and configure the
decoder then.

When this happens we log an error; it will show up in chrome://gpu (and
stderr).

This does mean that we are delaying decode failure (possibly indefinitely)
in that case.

(cherry picked from commit 8a2f5658c6301691342e302ea618c61cad5d220c)

BUG= 229412 , 583638 , 583698 , 516039 
TBR=dalecurtis@chromium.org

Review URL: https://codereview.chromium.org/1707843002 .

Cr-Commit-Position: refs/branch-heads/2623@{#432}
Cr-Branched-From: 92d77538a86529ca35f9220bd3cd512cbea1f086-refs/heads/master@{#369907}

[modify] http://crrev.com/75bd1db3ad32a3e3439a5811ff461581d4a037e3/content/common/gpu/media/vt_video_decode_accelerator_mac.cc
[modify] http://crrev.com/75bd1db3ad32a3e3439a5811ff461581d4a037e3/content/common/gpu/media/vt_video_decode_accelerator_mac.h

Comment 71 by amineer@chromium.org, Feb 17 2016

Cc: -amineer@chromium.org

Comment 72 by bugdroid1@chromium.org, Feb 17 2016

Project Member

Comment 73 by bugdroid1@chromium.org, Feb 24 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb

commit 250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb
Author: wolenetz <wolenetz@chromium.org>
Date: Wed Feb 24 20:27:09 2016

Reland: MSE: Relax the 'media segment must begin with keyframe' requirement

The current version of the MSE spec no longer requires that media segments
start with a keyframe. This change reworks the MSE implementation
so that it now allows tracks within media segments to not begin
with a keyframe.

To accomplish this requirement's relaxation, this CL:
1) Separates media segment signaling from coded frame group signaling:
   MSE stream parsers need to report whether or not they are currently
   in the middle of parsing a media segment, such as a WebM cluster, to
   comply with spec disallowance of certain operations like changing
   SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT.
   However, SourceBufferStream only needs to be signaled when a new coded
   frame group is beginning, for example when buffers are appended
   following a discontinuity.
2) Since the FrameProcessor now can transitively signal SourceBufferStreams
   when a new coded frame group is beginning (which might span multiple
   media segments), the SourceBufferStream can rely upon the fact
   that the next buffer appended must be a keyframe if it's been told
   that a new coded frame group is starting. A CHECK() is included.
3) New ChunkDemuxerTests.
4) Various test cleanup to conform to the distinct "new coded frame group"
   and "new media segment" signaling separation, as well as removal
   of decode error verification when a media segment doesn't begin
   with a keyframe.
5) Fixes regression found in previous attempt, tracked in  bug 581125 . In
   short, relaxes the restriction on same timestamp sequences. A
   MediaLog is added where decode error would occur previously.
6) Fixes regression found in previous attempt, tracked in  bug 581458 . In
   short, prevents tiny gaps under the FrameProcessor's discontinuity
   threshold from triggering buffered range gaps in SourceBufferStream if
   there is an intervening removal of media from the range last appended
   to, but the last appended GOP survived the removal.
7) Uses coded frame group start time more intelligently to determine
   range adjacency on appends. This allows extremely jagged start
   coded frame group appends to overlap an existing range, fixing a
   DCHECK which could occur otherwise (and adhering more correctly
   to muxed buffered range expectations.) Improves handling of range
   removals that may occur between SBS::OnStartOfCodedFrameGroup()
   and SBS::Append().
Note that the FrameProcessor already drops non-keyframes prior to the
first keyframe processed for a track following a discontinuity,
and the SourceBufferStream already drops non-keyframes prior to the
first keyframe processed for a track following a range removal that
removed the last appended position in the current coded frame group.

BUG= 229412 , 459546 , 581125 , 581458 , 462575 
R=chcunningham@chromium.org
TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests,
FrameProcessorTests. Also locally tested:  bug 459546 's
original report no longer fails. A sample webm VP9 file
from b/26524063 no longer fails. GPM webm vp9/opus test no
longer fails.  Bug 518854 's original report's "flags_all.mp4"
no longer fails. Bugs  581125  and  581458  no longer repro.

Review URL: https://codereview.chromium.org/1670033002

Cr-Commit-Position: refs/heads/master@{#377368}

[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/chunk_demuxer.cc
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/chunk_demuxer.h
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/chunk_demuxer_unittest.cc
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/frame_processor.cc
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/frame_processor.h
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/media_source_state.cc
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/media_source_state.h
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/source_buffer_range.cc
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/source_buffer_range.h
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/source_buffer_stream.h
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/filters/source_buffer_stream_unittest.cc
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/formats/mp2t/mp2t_stream_parser.cc
[modify] https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb/media/formats/mp4/mp4_stream_parser.cc

Comment 74 by wolenetz@chromium.org, Feb 24 2016

 Issue 518854  has been merged into this issue.

Comment 75 by wolenetz@chromium.org, Feb 26 2016

Labels: -M-45 -Merge-Merged-2623 -Merge-Merged-2454 M-50
Status: Fixed (was: Started)
Clearing the merge labels, since they were tracking the merge of a MEDIA_LOG message (#27), and the merge of a related Mac decoder fix (#70), not the actual fix that relaxes the keyframe requirement that has now (re)landed in ToT (#73) for M-50.

Comment 76 by sabrangu...@gmail.com, Feb 26 2016

Has any one has an estimate of when this will be fixed? Thanks!

Comment 77 by wolenetz@chromium.org, Feb 27 2016

@#76 it is fixed in Chromium tip of tree, confirmed in today's canaries. It should be in M-50 release.

Comment 78 by wolenetz@chromium.org, Feb 27 2016

Also, @#76, https://www.chromium.org/developers/calendar may also have more information (albeit estimated) for you.

Comment 79 by sabrangu...@gmail.com, Feb 27 2016

Nice, thanks a lot!

Comment 80 by bugdroid1@chromium.org, Sep 5

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1240638ead7a9b3568b4a30918aff44611c00208

commit 1240638ead7a9b3568b4a30918aff44611c00208
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Sep 05 21:40:56 2018

MSE: Use video (AVC) keyframeness when it differs from MP4's

Due to prevalence of MSE support in other browsers for legacy
incorrectly muxed content where AVC keyframeness is incorrectly marked
in the MP4 container, this change expands the previous
chrome://media-internals logging when such mismatch is detected, to
trust the keyframe-ness of the AVC coded frame for buffering and
decoding.  Otherwise, such streams, especially those with frames
incorrectly marked as keyframes in the MP4, would be buffered as
out-of-order GOPS rather than out-of-order nonkeyframes, corrupting the
buffering and decode sequence on playback in the new, compliant,
MseBufferByPts logic.  Seek preroll from actual AVC nonkeyframes in
legacy MseBufferByDts logic should also be fixed by this change.

BUG= 879734 , 844799 , 229412 ,760264

Change-Id: I90f53ec333e1aeeeaf39e96e176438cb13b4a195
Reviewed-on: https://chromium-review.googlesource.com/1205640
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589009}
[modify] https://crrev.com/1240638ead7a9b3568b4a30918aff44611c00208/media/formats/mp4/mp4_stream_parser.cc
[modify] https://crrev.com/1240638ead7a9b3568b4a30918aff44611c00208/media/formats/mp4/mp4_stream_parser_unittest.cc

Comment 81 by bugdroid1@chromium.org, Sep 12

Project Member
Labels: merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc67c41a328c4564c3e39d087b1510b28b32abce

commit fc67c41a328c4564c3e39d087b1510b28b32abce
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Sep 12 18:54:47 2018

To M70: MSE: Use video (AVC) keyframeness when it differs from MP4's

Due to prevalence of MSE support in other browsers for legacy
incorrectly muxed content where AVC keyframeness is incorrectly marked
in the MP4 container, this change expands the previous
chrome://media-internals logging when such mismatch is detected, to
trust the keyframe-ness of the AVC coded frame for buffering and
decoding.  Otherwise, such streams, especially those with frames
incorrectly marked as keyframes in the MP4, would be buffered as
out-of-order GOPS rather than out-of-order nonkeyframes, corrupting the
buffering and decode sequence on playback in the new, compliant,
MseBufferByPts logic.  Seek preroll from actual AVC nonkeyframes in
legacy MseBufferByDts logic should also be fixed by this change.

BUG= 879734 , 844799 , 229412 ,760264
TBR=dalecurtis@chromium.org,sandersd@chromium.org

Change-Id: I90f53ec333e1aeeeaf39e96e176438cb13b4a195
Reviewed-on: https://chromium-review.googlesource.com/1205640
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#589009}(cherry picked from commit 1240638ead7a9b3568b4a30918aff44611c00208)
Reviewed-on: https://chromium-review.googlesource.com/1222197
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#337}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/fc67c41a328c4564c3e39d087b1510b28b32abce/media/formats/mp4/mp4_stream_parser.cc
[modify] https://crrev.com/fc67c41a328c4564c3e39d087b1510b28b32abce/media/formats/mp4/mp4_stream_parser_unittest.cc

Sign in to add a comment