Commit e575361d authored by annacc@chromium.org's avatar annacc@chromium.org

Don't clear the list of source buffers in ChunkDemuxer::Shutdown()

Clearing the stream_parser_map_ (which represents the list of source buffers) creates an inconsistent state where source buffers should still be available to WebKit (JavaScript) but they have been removed internally. Source buffers should only be removed when RemoveId() is called or upon destruction. 

This change was originally committed as http://src.chromium.org/viewvc/chrome?view=rev&revision=148149 (Patch Set 1) and then reverted because it exposed a memory leak. Turns out the StreamParser was holding on to a reference to the ChunkDemuxer until after the initialization in order to ensure the init callback got called. So for tests that never append an initialization segment, the StreamParser reference prevented the ChunkDemuxer from being deconstructed, causing the memory leak. Before this patch, Shutdown() would remove all the StreamParsers so this wasn't a problem. 

A subsequent patch will not allow StreamParser to hang on to its ChunkDemuxer reference. This is ok because if ChunkDemuxer gets deconstructed, the callback is no longer relevant. 

For reference, the memory leak looks like this: 
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28valgrind%29%281%29/builds/11523/steps/memory%20test%3A%20media/logs/stdio 

BUG=138578
, WK88949 
TEST=http/tests/media/media-source/video-media-source-add-and-remove-ids.html, ChunkDemuxerTest.TestEndOfStreamWithNoAppends


Review URL: https://chromiumcodereview.appspot.com/10822024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@148473 0039d316-1c4b-4281-b951-d872f2087c98
parent b1361f0a
...@@ -655,7 +655,7 @@ ChunkDemuxer::Status ChunkDemuxer::AddId(const std::string& id, ...@@ -655,7 +655,7 @@ ChunkDemuxer::Status ChunkDemuxer::AddId(const std::string& id,
CHECK(stream_parser.get()); CHECK(stream_parser.get());
stream_parser->Init( stream_parser->Init(
base::Bind(&ChunkDemuxer::OnStreamParserInitDone, this), base::Bind(&ChunkDemuxer::OnStreamParserInitDone, base::Unretained(this)),
base::Bind(&ChunkDemuxer::OnNewConfigs, base::Unretained(this), base::Bind(&ChunkDemuxer::OnNewConfigs, base::Unretained(this),
has_audio, has_video), has_audio, has_video),
audio_cb, audio_cb,
...@@ -857,12 +857,6 @@ void ChunkDemuxer::Shutdown() { ...@@ -857,12 +857,6 @@ void ChunkDemuxer::Shutdown() {
if (video_) if (video_)
video_->Shutdown(); video_->Shutdown();
for (StreamParserMap::iterator it = stream_parser_map_.begin();
it != stream_parser_map_.end(); ++it) {
delete it->second;
}
stream_parser_map_.clear();
ChangeState_Locked(SHUTDOWN); ChangeState_Locked(SHUTDOWN);
} }
......
...@@ -905,6 +905,21 @@ TEST_F(ChunkDemuxerTest, TestEOSDuringInit) { ...@@ -905,6 +905,21 @@ TEST_F(ChunkDemuxerTest, TestEOSDuringInit) {
} }
TEST_F(ChunkDemuxerTest, TestEndOfStreamWithNoAppend) { TEST_F(ChunkDemuxerTest, TestEndOfStreamWithNoAppend) {
EXPECT_CALL(*client_, DemuxerOpened(_));
demuxer_->Initialize(
&host_, NewExpectedStatusCB(DEMUXER_ERROR_COULD_NOT_OPEN));
ASSERT_EQ(AddId(), ChunkDemuxer::kOk);
CheckExpectedRanges("{ }");
demuxer_->EndOfStream(PIPELINE_OK);
ShutdownDemuxer();
CheckExpectedRanges("{ }");
demuxer_->RemoveId(kSourceId);
demuxer_ = NULL;
}
TEST_F(ChunkDemuxerTest, TestEndOfStreamWithNoMediaAppend) {
ASSERT_TRUE(InitDemuxer(true, true, false)); ASSERT_TRUE(InitDemuxer(true, true, false));
CheckExpectedRanges("{ }"); CheckExpectedRanges("{ }");
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment