Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(24)

Issue 2491043003: MediaResource refactoring to support multiple streams (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 months, 2 weeks ago by servolk
Modified:
6 months ago
CC:
chromium-reviews, qsr+mojo_chromium.org, Aaron Boodman, posciak+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, feature-media-reviews_chromium.org, alokp+watch_chromium.org, darin (slow to review)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MediaResource refactoring to support multiple streams This CL changes the old GetStream method returning a single stream into a new GetAllStreams returning all available demuxer streams. BUG=487288 Review-Url: https://codereview.chromium.org/2491043003 Cr-Commit-Position: refs/heads/master@{#450819} Committed: https://chromium.googlesource.com/chromium/src/+/8acf5dcbc4e83fad7f23eb0380e0635058a636bc

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : rebase #

Patch Set 4 : rebase #

Patch Set 5 : rebase #

Patch Set 6 : buildfix #

Patch Set 7 : Chromecast buildfixes #

Patch Set 8 : buildfix #

Patch Set 9 : rebase #

Patch Set 10 : rebase #

Patch Set 11 : rebase #

Patch Set 12 : rebase #

Patch Set 13 : rebase #

Patch Set 14 : updated comment #

Patch Set 15 : Rename DemuxerStreamProvider -> MediaResource #

Patch Set 16 : fixed build breaks #

Patch Set 17 : rebase #

Total comments: 18

Patch Set 18 : CR feedback #

Patch Set 19 : rebase #

Patch Set 20 : cleanup #

Patch Set 21 : Move stream status notifications to MediaResource #

Patch Set 22 : rebase #

Patch Set 23 : cast build fix #

Patch Set 24 : Remove SetStreamStatusChangeCB from DemuxerStreamForTest #

Patch Set 25 : rebase #

Total comments: 42

Patch Set 26 : Leave only GetStream -> GetStreams changes in this CL #

Patch Set 27 : Extract DemuxerStreamProvider -> MediaResource rename into a separate CL #

Patch Set 28 : rebase #

Patch Set 29 : Reintroduc GetFirstStream API and return only enabled streams from GetAllStreams #

Total comments: 5

Patch Set 30 : Added a TODO about DemuxerStream enabled/set_enabled methods #

Unified diffs Side-by-side diffs Delta from patch set Stats (+462 lines, -413 lines) Patch
M chromecast/media/cma/base/demuxer_stream_for_test.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 26 1 chunk +0 lines, -1 line 0 comments Download
M chromecast/media/cma/base/demuxer_stream_for_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 26 1 chunk +0 lines, -5 lines 0 comments Download
M chromecast/media/cma/test/frame_segmenter_for_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +3 lines, -2 lines 0 comments Download
M chromecast/media/service/cast_renderer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +7 lines, -4 lines 0 comments Download
M media/base/demuxer_perftest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +3 lines, -13 lines 0 comments Download
M media/base/demuxer_stream.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 2 chunks +3 lines, -7 lines 0 comments Download
M media/base/fake_demuxer_stream.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +2 lines, -2 lines 0 comments Download
M media/base/fake_demuxer_stream.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +10 lines, -10 lines 0 comments Download
M media/base/fake_text_track_stream.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 26 1 chunk +0 lines, -1 line 0 comments Download
M media/base/fake_text_track_stream.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 26 1 chunk +0 lines, -5 lines 0 comments Download
M media/base/media_resource.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 5 chunks +25 lines, -9 lines 0 comments Download
M media/base/media_resource.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +9 lines, -0 lines 0 comments Download
M media/base/media_url_demuxer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +2 lines, -1 line 0 comments Download
M media/base/media_url_demuxer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +7 lines, -2 lines 0 comments Download
M media/base/mock_filters.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +3 lines, -2 lines 0 comments Download
M media/base/pipeline_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +17 lines, -16 lines 0 comments Download
M media/base/pipeline_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +6 lines, -4 lines 0 comments Download
M media/filters/chunk_demuxer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +4 lines, -2 lines 0 comments Download
M media/filters/chunk_demuxer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +19 lines, -19 lines 0 comments Download
M media/filters/chunk_demuxer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 43 chunks +93 lines, -80 lines 0 comments Download
M media/filters/decrypting_demuxer_stream.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 26 1 chunk +0 lines, -1 line 0 comments Download
M media/filters/decrypting_demuxer_stream.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 26 1 chunk +0 lines, -5 lines 0 comments Download
M media/filters/ffmpeg_demuxer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +8 lines, -5 lines 0 comments Download
M media/filters/ffmpeg_demuxer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 4 chunks +28 lines, -14 lines 0 comments Download
M media/filters/ffmpeg_demuxer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 46 chunks +90 lines, -78 lines 0 comments Download
M media/mojo/clients/mojo_renderer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +11 lines, -24 lines 0 comments Download
M media/mojo/clients/mojo_renderer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 4 chunks +7 lines, -5 lines 0 comments Download
M media/mojo/services/media_resource_shim.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +2 lines, -1 line 0 comments Download
M media/mojo/services/media_resource_shim.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +9 lines, -8 lines 0 comments Download
M media/mojo/services/mojo_demuxer_stream_adapter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 26 1 chunk +0 lines, -1 line 0 comments Download
M media/mojo/services/mojo_demuxer_stream_adapter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 26 1 chunk +0 lines, -5 lines 0 comments Download
M media/remoting/courier_renderer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +14 lines, -8 lines 0 comments Download
M media/remoting/fake_media_resource.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +2 lines, -2 lines 0 comments Download
M media/remoting/fake_media_resource.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +10 lines, -5 lines 0 comments Download
M media/renderers/renderer_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 4 chunks +19 lines, -16 lines 0 comments Download
M media/renderers/renderer_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 13 chunks +44 lines, -46 lines 0 comments Download
M media/test/pipeline_integration_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 4 chunks +5 lines, -4 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 144 (119 generated)
tguilbert
LGTM % nits. https://codereview.chromium.org/2491043003/diff/320001/chromecast/media/cma/test/frame_segmenter_for_test.cc File chromecast/media/cma/test/frame_segmenter_for_test.cc (right): https://codereview.chromium.org/2491043003/diff/320001/chromecast/media/cma/test/frame_segmenter_for_test.cc#newcode329 chromecast/media/cma/test/frame_segmenter_for_test.cc:329: if (audio && s->type() == ::media::DemuxerStream::AUDIO) ...
6 months, 4 weeks ago (2017-01-24 23:24:41 UTC) #66
servolk
https://codereview.chromium.org/2491043003/diff/320001/chromecast/media/cma/test/frame_segmenter_for_test.cc File chromecast/media/cma/test/frame_segmenter_for_test.cc (right): https://codereview.chromium.org/2491043003/diff/320001/chromecast/media/cma/test/frame_segmenter_for_test.cc#newcode329 chromecast/media/cma/test/frame_segmenter_for_test.cc:329: if (audio && s->type() == ::media::DemuxerStream::AUDIO) { On 2017/01/24 ...
6 months, 4 weeks ago (2017-01-25 00:57:32 UTC) #69
tguilbert
https://codereview.chromium.org/2491043003/diff/320001/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2491043003/diff/320001/media/filters/ffmpeg_demuxer.cc#newcode1039 media/filters/ffmpeg_demuxer.cc:1039: FFmpegDemuxerStream* FFmpegDemuxer::GetFFmpegStream( On 2017/01/25 00:57:32, servolk wrote: > On ...
6 months, 4 weeks ago (2017-01-25 02:36:46 UTC) #74
xhwang
Thanks for the CL! Could you please do a rename-only CL first, which should be ...
6 months, 2 weeks ago (2017-02-01 18:26:04 UTC) #101
servolk
On 2017/02/01 18:26:04, xhwang_slow wrote: > Thanks for the CL! > > Could you please ...
6 months, 2 weeks ago (2017-02-01 19:09:25 UTC) #102
servolk
https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/service/cast_renderer.cc File chromecast/media/service/cast_renderer.cc (right): https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/service/cast_renderer.cc#newcode94 chromecast/media/service/cast_renderer.cc:94: // Obtain the first enabled audio and video streams. ...
6 months, 2 weeks ago (2017-02-01 22:29:21 UTC) #105
servolk
On 2017/02/01 19:09:25, servolk wrote: > On 2017/02/01 18:26:04, xhwang_slow wrote: > > Thanks for ...
6 months, 2 weeks ago (2017-02-02 00:47:25 UTC) #107
xhwang
Thank you so much for splitting the CL! Before we get into details again, I'd ...
6 months, 2 weeks ago (2017-02-02 19:24:28 UTC) #108
servolk
On 2017/02/02 19:24:28, xhwang_slow wrote: > Thank you so much for splitting the CL! > ...
6 months, 1 week ago (2017-02-11 00:32:57 UTC) #113
xhwang
On 2017/02/11 00:32:57, servolk wrote: > On 2017/02/02 19:24:28, xhwang_slow wrote: > > Thank you ...
6 months, 1 week ago (2017-02-11 09:44:19 UTC) #114
xhwang
On 2017/02/11 00:32:57, servolk wrote: > On 2017/02/02 19:24:28, xhwang_slow wrote: > > Thank you ...
6 months, 1 week ago (2017-02-11 09:44:24 UTC) #115
servolk
On 2017/02/11 09:44:24, xhwang_slow wrote: > On 2017/02/11 00:32:57, servolk wrote: > > On 2017/02/02 ...
6 months, 1 week ago (2017-02-13 18:56:07 UTC) #116
xhwang
On 2017/02/13 18:56:07, servolk wrote: > On 2017/02/11 09:44:24, xhwang_slow wrote: > > On 2017/02/11 ...
6 months, 1 week ago (2017-02-13 21:08:21 UTC) #117
servolk
On 2017/02/13 21:08:21, xhwang_slow wrote: > On 2017/02/13 18:56:07, servolk wrote: > > On 2017/02/11 ...
6 months, 1 week ago (2017-02-14 21:59:18 UTC) #123
tguilbert
Still LGTM % one question/comment https://codereview.chromium.org/2491043003/diff/560001/media/base/pipeline_impl.cc File media/base/pipeline_impl.cc (right): https://codereview.chromium.org/2491043003/diff/560001/media/base/pipeline_impl.cc#newcode863 media/base/pipeline_impl.cc:863: if (demuxer_->GetAllStreams().empty()) { In ...
6 months, 1 week ago (2017-02-14 22:13:29 UTC) #124
servolk
https://codereview.chromium.org/2491043003/diff/560001/media/base/pipeline_impl.cc File media/base/pipeline_impl.cc (right): https://codereview.chromium.org/2491043003/diff/560001/media/base/pipeline_impl.cc#newcode863 media/base/pipeline_impl.cc:863: if (demuxer_->GetAllStreams().empty()) { On 2017/02/14 22:13:28, tguilbert wrote: > ...
6 months, 1 week ago (2017-02-15 00:03:25 UTC) #125
xhwang
LG, just have one question. https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_stream.h File media/base/demuxer_stream.h (right): https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_stream.h#newcode105 media/base/demuxer_stream.h:105: virtual void set_enabled(bool enabled, ...
6 months, 1 week ago (2017-02-15 05:32:19 UTC) #126
servolk
https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_stream.h File media/base/demuxer_stream.h (right): https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_stream.h#newcode105 media/base/demuxer_stream.h:105: virtual void set_enabled(bool enabled, base::TimeDelta timestamp) = 0; On ...
6 months ago (2017-02-15 17:39:16 UTC) #127
xhwang
https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_stream.h File media/base/demuxer_stream.h (right): https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_stream.h#newcode105 media/base/demuxer_stream.h:105: virtual void set_enabled(bool enabled, base::TimeDelta timestamp) = 0; On ...
6 months ago (2017-02-15 17:43:53 UTC) #128
servolk
On 2017/02/15 17:43:53, xhwang_slow wrote: > https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_stream.h > File media/base/demuxer_stream.h (right): > > https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_stream.h#newcode105 > ...
6 months ago (2017-02-15 18:33:38 UTC) #129
xhwang
Thank you for the patience! LGTM
6 months ago (2017-02-15 19:20:42 UTC) #132
servolk
On 2017/02/15 19:20:42, xhwang_slow wrote: > Thank you for the patience! LGTM Thanks for the ...
6 months ago (2017-02-15 19:43:48 UTC) #134
slan
On 2017/02/15 19:43:48, servolk wrote: > On 2017/02/15 19:20:42, xhwang_slow wrote: > > Thank you ...
6 months ago (2017-02-15 21:14:46 UTC) #137
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2491043003/580001
6 months ago (2017-02-15 21:18:04 UTC) #140
commit-bot: I haz the power
6 months ago (2017-02-15 22:43:54 UTC) #144
Message was sent while issue was closed.
Committed patchset #30 (id:580001) as
https://chromium.googlesource.com/chromium/src/+/8acf5dcbc4e83fad7f23eb0380e0...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b