Issue 348623003: Fix muxed MP4 parsing so it won't crash on partial media segment appends.

Issue 348623003: Fix muxed MP4 parsing so it won't crash on partial media segment appends. (Closed)

Created:
6 years, 6 months ago by acolwell GONE FROM CHROMIUM

Modified:
6 years, 6 months ago

Reviewers:
wolenetz

CC:
chromium-reviews, feature-media-reviews_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Fix muxed MP4 parsing so it won't crash on partial media segment appends. Partial appends could cause the MP4StreamParser to emit buffers in non-monotonically increasing timestamp order because of how we process trun boxes. This patch makes sure that processing of trun boxes is deferred until we have all the sample & aux_info data before emitting samples. This prevents the parser from emitting samples in such a way that will break downstream code. This is a minimal impact fix to avoid bad behavior at the expense of buffering more data. BUG=None TEST=All MP4StreamParserTests that do partial appends now verify the fix. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278650

Patch Set 1 #

Patch Set 2 : Make ChromeOS compiler happy #

Total comments: 23

Patch Set 3 : Address CR comments. #

Patch Set 4 : Fix compiler error and test #

Patch Set 5 : Fix run skipping bug. #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+106 lines, -15 lines)			Patch
M	media/formats/mp4/mp4_stream_parser.h	View		3 chunks	+15 lines, -0 lines	0 comments	Download
M	media/formats/mp4/mp4_stream_parser.cc	View	1 2 3 4	6 chunks	+68 lines, -13 lines	0 comments	Download
M	media/formats/mp4/mp4_stream_parser_unittest.cc	View	1 2 3	5 chunks	+23 lines, -2 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

wolenetz

Looking pretty good. https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_stream_parser.cc File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_stream_parser.cc#newcode80 media/formats/mp4/mp4_stream_parser.cc:80: Reset(); Before Reset(), we now have ...

6 years, 6 months ago (2014-06-19 22:01:15 UTC) #2

acolwell GONE FROM CHROMIUM

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_stream_parser.cc File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_stream_parser.cc#newcode80 media/formats/mp4/mp4_stream_parser.cc:80: Reset(); On 2014/06/19 22:01:14, wolenetz_OOO_June20-June29 wrote: > Before Reset(), ...

6 years, 6 months ago (2014-06-19 23:46:47 UTC) #3

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser.cc:80: Reset();
On 2014/06/19 22:01:14, wolenetz_OOO_June20-June29 wrote:
> Before Reset(), we now have the possibility we're still in
> kWaitingForSampleData, and there is a partial segment enqueued but not yet
> parsed or emitted that we should parse and emit on Flush() (maybe for even a
> subset of tracks). EnqueueSample(...)[perhaps repeatedly?] +
> SendAndFlushSamples() prior to Reset() here?

As discussed offline, I think we should just drop them since we run into the
same problems that partial appends have. We could end up appending a bunch of
data for a single track, but nothing from the other ones. I think that could be
real confusing since the pipeline would stall eventhough it looks like there is
buffered data.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser.cc:99: switch(state_) {
On 2014/06/19 22:01:14, wolenetz_OOO_June20-June29 wrote:
> lint nit: insert space before (

Done.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser.cc:104: 
On 2014/06/19 22:01:14, wolenetz_OOO_June20-June29 wrote:
> nit: add break; ? I'm not sure of code style here (guides don't clarify).
I don't believe it is necessary if all paths in the case return.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser.cc:421: // Remain in kEnqueueingSamples
state, discarding data, until the end of
On 2014/06/19 22:01:14, wolenetz_OOO_June20-June29 wrote:
> nit: seems a good time to fix this comment: s/kEnqueue/kEmitt/

Done.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser.cc:448: runs_->AdvanceRun();
On 2014/06/19 22:01:14, wolenetz_OOO_June20-June29 wrote:
> I'm confused. What if there isn't another run here? Seems we might need to
check
> !runs_->IsRunValid() before proceeding after this AdvanceRun() (by just
> returning true and letting the caller re-call us).

Yeah. I saw this too. I was planning on fixing that in a different CL because I
didn't have a test case for exposing the bug yet.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser.cc:601: // so we can ensure proper equeuing
behavior. Otherwise assume we have enough
On 2014/06/19 22:01:15, wolenetz_OOO_June20-June29 wrote:
> nit: s/eq/enq

Done.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser.cc:610: highest_end_offset_ = 0;
On 2014/06/19 22:01:14, wolenetz_OOO_June20-June29 wrote:
> nit: Can we safely short-cut and return true after setting 0 here if there is
> only one track_id for which we're interested in emitting samples (e.g.
> audio-only or video-only)?
I suppose, but that would then mean that highest_end_offset_ would not have a
consistent definition. ISTM that would be rather confusing.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser_unittest.cc:34: : configs_received_(false) {
On 2014/06/19 22:01:15, wolenetz_OOO_June20-June29 wrote:
> nit: initialize lower_bound_ to base::TimeDelta::Max() to cause any
> NewBuffersF() before first NewSegmentF() to fail?

Done.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser_unittest.cc:100: 
On 2014/06/19 22:01:15, wolenetz_OOO_June20-June29 wrote:
> nit: remove extra whitespace

Done.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser_unittest.cc:135: DVLOG(1) <<
"EndOfSegmentF()";
On 2014/06/19 22:01:15, wolenetz_OOO_June20-June29 wrote:
> nit: set lower_bound_ = base::TimeDelta::Max() to cause any NewBuffersF()
before
> next NewSegmentF() to fail?

Done.

wolenetz

lgtm % 2 nits: nit: s/buffing/buffering/ in CL description. (Thank you for fixing this nasty ...

6 years, 6 months ago (2014-06-20 00:17:29 UTC) #4

lgtm % 2 nits:

nit: s/buffing/buffering/ in CL description.

(Thank you for fixing this nasty potential blocker of using the shiny new
FrameProcessor!)

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser.cc:80: Reset();
On 2014/06/19 23:46:46, acolwell wrote:
> On 2014/06/19 22:01:14, wolenetz_OOO_June20-June29 wrote:
> > Before Reset(), we now have the possibility we're still in
> > kWaitingForSampleData, and there is a partial segment enqueued but not yet
> > parsed or emitted that we should parse and emit on Flush() (maybe for even a
> > subset of tracks). EnqueueSample(...)[perhaps repeatedly?] +
> > SendAndFlushSamples() prior to Reset() here?
> 
> As discussed offline, I think we should just drop them since we run into the
> same problems that partial appends have. We could end up appending a bunch of
> data for a single track, but nothing from the other ones. I think that could
be
> real confusing since the pipeline would stall eventhough it looks like there
is
> buffered data.

I agree.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser.cc:104: 
On 2014/06/19 23:46:46, acolwell wrote:
> On 2014/06/19 22:01:14, wolenetz_OOO_June20-June29 wrote:
> > nit: add break; ? I'm not sure of code style here (guides don't clarify).
> I don't believe it is necessary if all paths in the case return.

Yeah, it was just a style question, not a correctness question. I'm fine without
break; added in this case.

https://codereview.chromium.org/348623003/diff/20001/media/formats/mp4/mp4_st...
media/formats/mp4/mp4_stream_parser.cc:448: runs_->AdvanceRun();
On 2014/06/19 23:46:46, acolwell wrote:
> On 2014/06/19 22:01:14, wolenetz_OOO_June20-June29 wrote:
> > I'm confused. What if there isn't another run here? Seems we might need to
> check
> > !runs_->IsRunValid() before proceeding after this AdvanceRun() (by just
> > returning true and letting the caller re-call us).
> 
> Yeah. I saw this too. I was planning on fixing that in a different CL because
I
> didn't have a test case for exposing the bug yet.

nit: (sgtm) Maybe add a TODO?

acolwell GONE FROM CHROMIUM

The CQ bit was checked by acolwell@chromium.org

6 years, 6 months ago (2014-06-20 00:26:39 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/348623003/70001

6 years, 6 months ago (2014-06-20 00:28:37 UTC) #6

Message was sent while issue was closed.

Change committed as 278650

Expand Messages | Collapse Messages