Issue 9968117: Move Demuxer::set_host() to Initialize().

Issue 9968117: Move Demuxer::set_host() to Initialize(). (Closed)

Created:
8 years, 8 months ago by scherkus (not reviewing)

Modified:
8 years, 8 months ago

Reviewers:
acolwell GONE FROM CHROMIUM

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

Base URL:
svn://chrome-svn/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Move Demuxer::set_host() to Initialize(). BUG=111585 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130842

Patch Set 2 : fixes #

Created: 8 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+187 lines, -233 lines)			Patch
M	media/base/demuxer.h	View	1	2 chunks	+5 lines, -20 lines	0 comments	Download
M	media/base/demuxer.cc	View		1 chunk	+1 line, -7 lines	0 comments	Download
M	media/base/mock_filters.h	View		2 chunks	+5 lines, -22 lines	0 comments	Download
M	media/base/mock_filters.cc	View		1 chunk	+1 line, -26 lines	0 comments	Download
M	media/base/pipeline.cc	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	media/base/pipeline_unittest.cc	View	1	16 chunks	+62 lines, -22 lines	0 comments	Download
M	media/filters/chunk_demuxer.h	View		2 chunks	+3 lines, -1 line	0 comments	Download
M	media/filters/chunk_demuxer.cc	View		5 chunks	+10 lines, -7 lines	0 comments	Download
M	media/filters/chunk_demuxer_unittest.cc	View		18 chunks	+26 lines, -24 lines	0 comments	Download
M	media/filters/dummy_demuxer.h	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	media/filters/dummy_demuxer.cc	View	1	2 chunks	+3 lines, -6 lines	0 comments	Download
M	media/filters/ffmpeg_demuxer.h	View		3 chunks	+5 lines, -3 lines	0 comments	Download
M	media/filters/ffmpeg_demuxer.cc	View	1	7 chunks	+20 lines, -14 lines	0 comments	Download
M	media/filters/ffmpeg_demuxer_unittest.cc	View	1	7 chunks	+43 lines, -77 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

scherkus (not reviewing)

as promised https://chromiumcodereview.appspot.com/9968117/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): https://chromiumcodereview.appspot.com/9968117/diff/1/media/base/demuxer.h#newcode44 media/base/demuxer.h:44: // to be released before the host ...

8 years, 8 months ago (2012-04-04 02:02:02 UTC) #1

acolwell GONE FROM CHROMIUM

LGTM % comments https://chromiumcodereview.appspot.com/9968117/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): https://chromiumcodereview.appspot.com/9968117/diff/1/media/base/demuxer.h#newcode44 media/base/demuxer.h:44: // to be released before the ...

8 years, 8 months ago (2012-04-04 16:01:00 UTC) #2

scherkus (not reviewing)

8 years, 8 months ago (2012-04-05 01:54:53 UTC) #3

http://codereview.chromium.org/9968117/diff/1/media/base/demuxer.h
File media/base/demuxer.h (right):

http://codereview.chromium.org/9968117/diff/1/media/base/demuxer.h#newcode44
media/base/demuxer.h:44: // to be released before the host object is destroyed
by the pipeline.
On 2012/04/04 16:01:00, acolwell wrote:
> On 2012/04/04 02:02:03, scherkus wrote:
> > I copied this comment from above but it confuses me.
> > 
> > Open to suggestions!
> I believe the original intent was to signal that a Demuxer should not hold a
> strong reference to DemuxerHost because the host already holds a strong
> reference to the demuxer and is guarenteed to outlive it. What are you
confused
> about?

I think it's because we haven't used the term "strong reference" very often.
I'll reword it in layman terms ("you don't own this pointer, it's guaranteed to
outlive you")

http://codereview.chromium.org/9968117/diff/1/media/filters/dummy_demuxer.cc
File media/filters/dummy_demuxer.cc (left):

http://codereview.chromium.org/9968117/diff/1/media/filters/dummy_demuxer.cc#...
media/filters/dummy_demuxer.cc:60:
host()->SetDuration(media::kInfiniteDuration());
On 2012/04/04 16:01:00, acolwell wrote:
> Why don't we need this call anymore?

whoops!

http://codereview.chromium.org/9968117/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/9968117/diff/1/media/filters/ffmpeg_demuxer.cc...
media/filters/ffmpeg_demuxer.cc:472: data_source_->set_host(host);
On 2012/04/04 16:01:00, acolwell wrote:
> This guy is next... ;)

http://crbug.com/122071 added

http://codereview.chromium.org/9968117/diff/1/media/filters/ffmpeg_demuxer_un...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/9968117/diff/1/media/filters/ffmpeg_demuxer_un...
media/filters/ffmpeg_demuxer_unittest.cc:590: demuxer->Initialize(&host_,
NewExpectedStatusCB(PIPELINE_OK));
On 2012/04/04 16:01:00, acolwell wrote:
> what?! Initialize() wasn't necessary before? Seems like we are missing a
DCHECK
> somewhere. ISTM that Reads before Initialize() should fail or trigger a DCHECK
> of some sort.

DCHECK(host_) added and rewrote these tests

Expand Messages | Collapse Messages