Issue 1735803002: Implemented passing media track info from ffmpeg into blink

Issue 1735803002: Implemented passing media track info from ffmpeg into blink (Closed)

Created:
4 years, 10 months ago by servolk

Modified:
4 years, 9 months ago

Reviewers:
philipj_slow, fs, wolenetz

CC:
blink-reviews, chromium-reviews, eric.carlson_apple.com, feature-media-reviews_chromium.org, gunsch+watch_chromium.org, halliwell+watch_chromium.org, lcwu+watch_chromium.org, mlamouri+watch-blink_chromium.org, chcunningham

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

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Implemented passing media track info from ffmpeg into blink This CL is a combination of: 1. Basic implementation of media tracks https://codereview.chromium.org/1716503002/ 2. Unification of media track updates on demuxer level https://codereview.chromium.org/1727243002/ 3. Reading media track metadata in ffmpeg and passing it to blink https://codereview.chromium.org/1735763002/

Patch Set 1 #

Total comments: 10

Patch Set 2 : Fixed cast_shell + test nits #

Total comments: 4

Patch Set 3 : wolenetz@ CR feedback + better track info extraction in ffmpeg #

Created: 4 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+662 lines, -165 lines)			Patch
M	chromecast/media/cma/test/frame_segmenter_for_test.cc	View	1	3 chunks	+6 lines, -1 line	0 comments	Download
M	media/base/BUILD.gn	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	media/base/demuxer.h	View		2 chunks	+6 lines, -0 lines	0 comments	Download
M	media/base/demuxer_perftest.cc	View		3 chunks	+9 lines, -1 line	0 comments	Download
A	media/base/media_track.h	View	1 2	1 chunk	+41 lines, -0 lines	0 comments	Download
A	media/base/media_track.cc	View	1 2	1 chunk	+18 lines, -0 lines	0 comments	Download
A	media/base/media_tracks.h	View	1 2	1 chunk	+62 lines, -0 lines	0 comments	Download
A	media/base/media_tracks.cc	View	1 2	1 chunk	+81 lines, -0 lines	0 comments	Download
M	media/base/stream_parser.h	View		2 chunks	+9 lines, -11 lines	0 comments	Download
M	media/blink/webmediaplayer_impl.h	View		3 chunks	+7 lines, -0 lines	0 comments	Download
M	media/blink/webmediaplayer_impl.cc	View		3 chunks	+45 lines, -1 line	0 comments	Download
M	media/blink/websourcebuffer_impl.h	View		2 chunks	+3 lines, -1 line	0 comments	Download
M	media/blink/websourcebuffer_impl.cc	View	1 2	4 chunks	+9 lines, -6 lines	0 comments	Download
M	media/filters/chunk_demuxer.h	View		2 chunks	+11 lines, -10 lines	0 comments	Download
M	media/filters/chunk_demuxer.cc	View		3 chunks	+16 lines, -13 lines	0 comments	Download
M	media/filters/chunk_demuxer_unittest.cc	View		37 chunks	+66 lines, -47 lines	0 comments	Download
M	media/filters/ffmpeg_demuxer.h	View		2 chunks	+3 lines, -0 lines	0 comments	Download
M	media/filters/ffmpeg_demuxer.cc	View	1 2	7 chunks	+25 lines, -0 lines	0 comments	Download
M	media/filters/ffmpeg_demuxer_unittest.cc	View	1 2	5 chunks	+32 lines, -1 line	0 comments	Download
M	media/filters/media_source_state.h	View		6 chunks	+13 lines, -12 lines	0 comments	Download
M	media/filters/media_source_state.cc	View	1 2	5 chunks	+29 lines, -16 lines	0 comments	Download
M	media/formats/common/stream_parser_test_base.h	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	media/formats/common/stream_parser_test_base.cc	View	1 2	2 chunks	+8 lines, -7 lines	0 comments	Download
M	media/formats/mp2t/mp2t_stream_parser.cc	View	1 2	4 chunks	+22 lines, -6 lines	0 comments	Download
M	media/formats/mp2t/mp2t_stream_parser_unittest.cc	View	1 2	2 chunks	+7 lines, -3 lines	0 comments	Download
M	media/formats/mp4/mp4_stream_parser.cc	View	1 2	2 chunks	+11 lines, -1 line	0 comments	Download
M	media/formats/mp4/mp4_stream_parser_unittest.cc	View	1 2	2 chunks	+8 lines, -6 lines	0 comments	Download
M	media/formats/mpeg/mpeg_audio_stream_parser_base.cc	View		2 chunks	+6 lines, -2 lines	0 comments	Download
M	media/formats/webm/webm_stream_parser.cc	View	1 2	2 chunks	+12 lines, -3 lines	0 comments	Download
M	media/media.gyp	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	media/test/pipeline_integration_test.cc	View		8 chunks	+15 lines, -12 lines	0 comments	Download
M	media/test/pipeline_integration_test_base.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	media/test/pipeline_integration_test_base.cc	View		3 chunks	+13 lines, -3 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html	View	1 2	1 chunk	+29 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/media/avtrack/video-track-properties.html	View	1 2	1 chunk	+29 lines, -0 lines	0 comments	Download

Messages

Total messages: 18 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

servolk

On 2016/02/25 02:31:23, servolk wrote: > mailto:servolk@chromium.org changed reviewers: > + mailto:chcunningham@chromium.org, mailto:fs@opera.com, mailto:philipj@opera.com, > ...

4 years, 10 months ago (2016-02-25 02:55:44 UTC) #3

On 2016/02/25 at 02:55:44, servolk wrote: > On 2016/02/25 02:31:23, servolk wrote: > > mailto:servolk@chromium.org ...

4 years, 10 months ago (2016-02-25 09:49:28 UTC) #4

https://codereview.chromium.org/1735803002/diff/1/third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html File third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html (right): https://codereview.chromium.org/1735803002/diff/1/third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html#newcode10 third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html:10: <div id="log"></div> You shouldn't need this. Ditto for <html>, ...

4 years, 10 months ago (2016-02-25 11:40:40 UTC) #5

servolk

4 years, 10 months ago (2016-02-25 18:14:23 UTC) #6

4 years, 10 months ago (2016-02-25 18:36:40 UTC) #7

philipj_slow

4 years, 10 months ago (2016-02-26 11:42:56 UTC) #8

https://codereview.chromium.org/1735803002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html
(right):

https://codereview.chromium.org/1735803002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html:10:
<div id="log"></div>
On 2016/02/25 18:36:40, fs wrote:
> On 2016/02/25 at 18:14:23, servolk wrote:
> > On 2016/02/25 11:40:39, fs wrote:
> > > You shouldn't need this. Ditto for <html>, <head> and <body>.
> > 
> > Are you sure? For some reason all other HTML files in this folder have them,
> so I'd prefer to keep those too, for consistency with other tests. If we
decide
> to remove all these extra tags, then we can do this later for all tests at
once
> in a separate CL.
> 
> I don't care greatly, but personally I think it's better to try to write new
> tests per existing guidelines/style [1] rather than maintaining some old style
> artifact. But personally I find this test (and all the other tests in this
> directory) to be way too deeply indented too so... =)
> 
> [1]
>
http://dev.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h...

FWIW, I always go with minimal markup and minimal indentation (basically only
indent inside script) when writing new testharness.js tests.

https://codereview.chromium.org/1735803002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html
(right):

https://codereview.chromium.org/1735803002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html:23:
assert_equals(e.audioTracks[0].language, "eng", "audioTrack.language");
Does test.mp4 actually have an audio track marked up as English, and if so why?
Also, why is it "eng" and not "en"?

https://codereview.chromium.org/1735803002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/avtrack/video-track-properties.html
(right):

https://codereview.chromium.org/1735803002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/avtrack/video-track-properties.html:23:
assert_equals(e.videoTracks[0].language, "eng", "videoTrack.language");
This seems even more strange. What does it mean for a video track to be in
English?

servolk

https://codereview.chromium.org/1735803002/diff/20001/third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html File third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html (right): https://codereview.chromium.org/1735803002/diff/20001/third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html#newcode23 third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html:23: assert_equals(e.audioTracks[0].language, "eng", "audioTrack.language"); On 2016/02/26 11:42:56, philipj_UTC7 wrote: > ...

4 years, 10 months ago (2016-02-27 00:56:21 UTC) #10

https://codereview.chromium.org/1735803002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html
(right):

https://codereview.chromium.org/1735803002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/avtrack/audio-track-properties.html:23:
assert_equals(e.audioTracks[0].language, "eng", "audioTrack.language");
On 2016/02/26 11:42:56, philipj_UTC7 wrote:
> Does test.mp4 actually have an audio track marked up as English, and if so
why?
> Also, why is it "eng" and not "en"?

I'm not sure how the test.mp4 was generated, but yes, it does actually have an
audio track marked "eng".
I've been working on better sourcing of media track data from for all media
track fields. As wolenetz@ said the spec for that is:
https://dev.w3.org/html5/html-sourcing-inband-tracks/#mpeg4
For this particular test.mp4 file the value of the language_code field in
MediaHeaderBox translates to "eng".

To clarify: I've first implemented better track info sourcing in our internal
mp4/webm parsers in a separate CL for MSE
(https://codereview.chromium.org/1735003004/), and then I did the same for
ffmpeg demuxer which provides all the same metadata that is available directly
to us in our internal mp4 parser. Note that in the MSE CL for a test file
generated by a different tool the values of track.label and track.language are
different, the language is "und" (i.e. undefined) there.

https://codereview.chromium.org/1735803002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/avtrack/video-track-properties.html
(right):

https://codereview.chromium.org/1735803002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/avtrack/video-track-properties.html:23:
assert_equals(e.videoTracks[0].language, "eng", "videoTrack.language");
On 2016/02/26 11:42:56, philipj_UTC7 wrote:
> This seems even more strange. What does it mean for a video track to be in
> English?

See my other comment. Per
https://dev.w3.org/html5/html-sourcing-inband-tracks/#mpeg4 spec both audio and
video tracks have the language property. And here the video track is marked as
'eng' as well.

philipj_slow

Sorry for not being explicit about the issue with "eng". AudioTrack.language and VideoTrack.language should be ...

4 years, 9 months ago (2016-03-02 11:11:45 UTC) #11

servolk

On 2016/03/02 11:11:45, philipj_UTC7 wrote: > Sorry for not being explicit about the issue with ...

4 years, 9 months ago (2016-03-03 06:09:02 UTC) #12

On 2016/03/02 11:11:45, philipj_UTC7 wrote:
> Sorry for not being explicit about the issue with "eng". AudioTrack.language
and
> VideoTrack.language should be BCP 47, and https://tools.ietf.org/html/bcp47
> says:
> 
>  language      = 2*3ALPHA            ; shortest ISO 639 code
>                  ["-" extlang]       ; sometimes followed by
>                                      ; extended language subtags
>                / 4ALPHA              ; or reserved for future use
>                / 5*8ALPHA            ; or registered language subtag
> 
> So as I read this, it should be "en", not "eng", because it's the shortest ISO
> 639 code for English. It looks like MPEG-4 uses just plain ISO 639, which does
> allow the longer form, obviously.
> 
> The other concern I have is that if the first file tested claims to have an
> English audio and video track, then it seems likely that some popular tool
uses
> English by default, which would make this API almost always wrong. It's worth
> investigating. Looking at test.mp4 I see the "mdhd" which is the
MediaHeaderBox,
> and 0x15c7 towards the end, confirmed by
> http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3930 based on
>
https://github.com/copiousfreetime/mp4parser/blob/master/isoparser/src/main/j...
> 
> So the string "eng" is really from the source, it's not a default that came
from
> somewhere :(
> 
> Finally, if https://dev.w3.org/html5/html-sourcing-inband-tracks/ says
something
> silly, it's almost certainly possible to change. I don't think it's been
> subjected to much scrutiny from browser implementors yet.

Interesting, do you know what spec specifies that language code should be BCP
47? I have found a few claims here and there that it should be, but haven't
found any official specs that mentioned BCP 47 specifically.
I agree that https://dev.w3.org/html5/html-sourcing-inband-tracks/ is far from
ideal, but I haven't found anything better yet. So now I was using it as the
spec for extracting track data. It specifically says that for MP4 ISO BMFF the
language is 'Content of the language field in the MediaHeaderBox'. So that's
what I implemented at the moment, and IIUC language field in MDHD box is always
3 chars (I've found that out by reading the MDHD description in
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFChap...).
If I understand correctly the full-form BCP47 language tag might be present in
MP4 ISO BMFF in an ELNG box, but it's optional. I've actually tried to use that
first (here is a CL that tries to read BCP47 lang code from ELNG tag in mp4
parser - https://codereview.chromium.org/1718773002/), but I put that on hold
for now, since none of our test streams have the ELNG tag at the moment.
And language codes are not the only problem. There's also problem with the
track.label property. The w3 spec suggests using 'Content of the name field in
the HandlerBox' for that, and some other tools use that as well. But after
reading it from our test files I can see that it's also not very user-friendly.
I have extracted the changes for reading media track metadata into a separate
CL:
https://codereview.chromium.org/1735003004/
Feel free to add your comments on this topic there.
I agree that some things can be improved, but it's good enough for now to
unblock further development of media tracks functionality. For now I'm planning
to focus more on getting basic media tracks functionality working end-to-end
(i.e. ensure that media track info is generated on Chromium level and passed to
blink, and that media track manipulations done by JS code, such as
enabling/disabling audio tracks, selecting video track, is correctly propagated
back to Chromium media pipeline and handled by it).

servolk

On 2016/03/03 06:09:02, servolk wrote: > On 2016/03/02 11:11:45, philipj_UTC7 wrote: > > Sorry for ...

4 years, 9 months ago (2016-03-03 06:18:32 UTC) #13

On 2016/03/03 06:09:02, servolk wrote:
> On 2016/03/02 11:11:45, philipj_UTC7 wrote:
> > Sorry for not being explicit about the issue with "eng". AudioTrack.language
> and
> > VideoTrack.language should be BCP 47, and https://tools.ietf.org/html/bcp47
> > says:
> > 
> >  language      = 2*3ALPHA            ; shortest ISO 639 code
> >                  ["-" extlang]       ; sometimes followed by
> >                                      ; extended language subtags
> >                / 4ALPHA              ; or reserved for future use
> >                / 5*8ALPHA            ; or registered language subtag
> > 
> > So as I read this, it should be "en", not "eng", because it's the shortest
ISO
> > 639 code for English. It looks like MPEG-4 uses just plain ISO 639, which
does
> > allow the longer form, obviously.
> > 
> > The other concern I have is that if the first file tested claims to have an
> > English audio and video track, then it seems likely that some popular tool
> uses
> > English by default, which would make this API almost always wrong. It's
worth
> > investigating. Looking at test.mp4 I see the "mdhd" which is the
> MediaHeaderBox,
> > and 0x15c7 towards the end, confirmed by
> > http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3930 based on
> >
>
https://github.com/copiousfreetime/mp4parser/blob/master/isoparser/src/main/j...
> > 
> > So the string "eng" is really from the source, it's not a default that came
> from
> > somewhere :(
> > 
> > Finally, if https://dev.w3.org/html5/html-sourcing-inband-tracks/ says
> something
> > silly, it's almost certainly possible to change. I don't think it's been
> > subjected to much scrutiny from browser implementors yet.
> 
> Interesting, do you know what spec specifies that language code should be BCP
> 47? I have found a few claims here and there that it should be, but haven't
> found any official specs that mentioned BCP 47 specifically.
> I agree that https://dev.w3.org/html5/html-sourcing-inband-tracks/ is far from
> ideal, but I haven't found anything better yet. So now I was using it as the
> spec for extracting track data. It specifically says that for MP4 ISO BMFF the
> language is 'Content of the language field in the MediaHeaderBox'. So that's
> what I implemented at the moment, and IIUC language field in MDHD box is
always
> 3 chars (I've found that out by reading the MDHD description in
>
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFChap...).
> If I understand correctly the full-form BCP47 language tag might be present in
> MP4 ISO BMFF in an ELNG box, but it's optional. I've actually tried to use
that
> first (here is a CL that tries to read BCP47 lang code from ELNG tag in mp4
> parser - https://codereview.chromium.org/1718773002/), but I put that on hold
> for now, since none of our test streams have the ELNG tag at the moment.
> And language codes are not the only problem. There's also problem with the
> track.label property. The w3 spec suggests using 'Content of the name field in
> the HandlerBox' for that, and some other tools use that as well. But after
> reading it from our test files I can see that it's also not very
user-friendly.
> I have extracted the changes for reading media track metadata into a separate
> CL:
> https://codereview.chromium.org/1735003004/
> Feel free to add your comments on this topic there.
> I agree that some things can be improved, but it's good enough for now to
> unblock further development of media tracks functionality. For now I'm
planning
> to focus more on getting basic media tracks functionality working end-to-end
> (i.e. ensure that media track info is generated on Chromium level and passed
to
> blink, and that media track manipulations done by JS code, such as
> enabling/disabling audio tracks, selecting video track, is correctly
propagated
> back to Chromium media pipeline and handled by it).

Oh, yes, and btw. I've put this big CL together to better illustrate the overall
picture of the changes that I'm trying to make. But I'm doing active development
of separate pieces of functionality related to this in separate smaller CLs
(wolenetz@ told me that he prefers to review these changes as separate CLs):
https://codereview.chromium.org/1727243002/
https://codereview.chromium.org/1735003004/
https://codereview.chromium.org/1658033002/
https://codereview.chromium.org/1735763002/
https://codereview.chromium.org/1659653002/

So this CL is already slightly out-of-date, but I can keep updating it with
merging changes of those smaller CLs, if you find it helpful.

philipj_slow

On 2016/03/03 06:18:32, servolk wrote: > Oh, yes, and btw. I've put this big CL ...

4 years, 9 months ago (2016-03-10 14:03:32 UTC) #14

wolenetz

On 2016/03/10 14:03:32, philipj_UTC7 wrote: > On 2016/03/03 06:18:32, servolk wrote: > > > Oh, ...

4 years, 9 months ago (2016-03-10 19:29:27 UTC) #15

servolk

On 2016/03/10 19:29:27, wolenetz wrote: > On 2016/03/10 14:03:32, philipj_UTC7 wrote: > > On 2016/03/03 ...

4 years, 9 months ago (2016-03-10 19:57:54 UTC) #16

On 2016/03/10 19:29:27, wolenetz wrote:
> On 2016/03/10 14:03:32, philipj_UTC7 wrote:
> > On 2016/03/03 06:18:32, servolk wrote:
> > 
> > > Oh, yes, and btw. I've put this big CL together to better illustrate the
> > overall
> > > picture of the changes that I'm trying to make. But I'm doing active
> > development
> > > of separate pieces of functionality related to this in separate smaller
CLs
> > > (wolenetz@ told me that he prefers to review these changes as separate
CLs):
> > > https://codereview.chromium.org/1727243002/
> > > https://codereview.chromium.org/1735003004/
> > > https://codereview.chromium.org/1658033002/
> > > https://codereview.chromium.org/1735763002/
> > > https://codereview.chromium.org/1659653002/
> > > 
> > > So this CL is already slightly out-of-date, but I can keep updating it
with
> > > merging changes of those smaller CLs, if you find it helpful.
> > 
> > I also like smaller CLs, so no need to keep a combined CL up to date. It
would
> > be useful if each individual CL links to all in-flight CLs, however, so that
a
> > reviewer could apply all of them locally and look at the combined result.
> 
> (+1 to the small CLs referencing the other small CLs known at the time.)

Well, as I've recently noticed myself, this information is already provided by
the CR tool, so there's probably no value in adding that explicitly in the CL
descriptions, so I've stopped doing that.

Take a closer look for example at the latest patchset of this CL:
https://codereview.chromium.org/1658033002/#ps350001

Under the trybot results you should see the link to the patchset of the base CL:

>Depends on Patchset:
>Issue 1659653002 Patch 210001

Now if you go to https://codereview.chromium.org/1659653002#ps210001, you will
see, again under the trybot results, that CL 1659653002#ps210001 is based on
735003004#ps220001 and serves as a base for the three latest patchsets of CL
1658033002.

>Depends on Patchset:
>Issue 1735003004 Patch 220001

>Dependent Patchsets:
>Issue 1658033002 Patch 310001
>Issue 1658033002 Patch 330001
>Issue 1658033002 Patch 350001

Using those links provided by the CL tools, you should be able to easily figure
out in just a few clicks that the CL for reading media track info
(https://codereview.chromium.org/1735003004/) depended on the CL that unified
passing media track info on the demuxer level (recently landed
https://codereview.chromium.org/1727243002/). The CLs for passing media track
info from FFmpeg streams into blink HTMLMediaElement
(https://codereview.chromium.org/1735803002/) and for passing media track info
from MSE/ChunkDemuxer into MSE SourceBuffer
(https://codereview.chromium.org/1659653002/) both depend on the CL for reading
media track info (https://codereview.chromium.org/1735003004/) but don't depend
on each other (since MSE/ChunkDemux and FFmpeg/direct URL playback are two
independent code paths). And the CL for introducing SourceBuffer specific media
track extension interfaces (https://codereview.chromium.org/1658033002/) depends
on the CL that implements passing of the basic track data from ChunkDemuxer into
blink SourceBuffer (https://codereview.chromium.org/1659653002/). Does it make
sense?

wolenetz

On 2016/03/10 19:57:54, servolk wrote: > On 2016/03/10 19:29:27, wolenetz wrote: > > On 2016/03/10 ...

4 years, 9 months ago (2016-03-10 20:06:47 UTC) #17

On 2016/03/10 19:57:54, servolk wrote:
> On 2016/03/10 19:29:27, wolenetz wrote:
> > On 2016/03/10 14:03:32, philipj_UTC7 wrote:
> > > On 2016/03/03 06:18:32, servolk wrote:
> > > 
> > > > Oh, yes, and btw. I've put this big CL together to better illustrate the
> > > overall
> > > > picture of the changes that I'm trying to make. But I'm doing active
> > > development
> > > > of separate pieces of functionality related to this in separate smaller
> CLs
> > > > (wolenetz@ told me that he prefers to review these changes as separate
> CLs):
> > > > https://codereview.chromium.org/1727243002/
> > > > https://codereview.chromium.org/1735003004/
> > > > https://codereview.chromium.org/1658033002/
> > > > https://codereview.chromium.org/1735763002/
> > > > https://codereview.chromium.org/1659653002/
> > > > 
> > > > So this CL is already slightly out-of-date, but I can keep updating it
> with
> > > > merging changes of those smaller CLs, if you find it helpful.
> > > 
> > > I also like smaller CLs, so no need to keep a combined CL up to date. It
> would
> > > be useful if each individual CL links to all in-flight CLs, however, so
that
> a
> > > reviewer could apply all of them locally and look at the combined result.
> > 
> > (+1 to the small CLs referencing the other small CLs known at the time.)
> 
> Well, as I've recently noticed myself, this information is already provided by
> the CR tool, so there's probably no value in adding that explicitly in the CL
> descriptions, so I've stopped doing that.
> 
> Take a closer look for example at the latest patchset of this CL:
> https://codereview.chromium.org/1658033002/#ps350001
> 
> Under the trybot results you should see the link to the patchset of the base
CL:
> 
> >Depends on Patchset:
> >Issue 1659653002 Patch 210001
> 
> Now if you go to https://codereview.chromium.org/1659653002#ps210001, you will
> see, again under the trybot results, that CL 1659653002#ps210001 is based on
> 735003004#ps220001 and serves as a base for the three latest patchsets of CL
> 1658033002.
> 
> >Depends on Patchset:
> >Issue 1735003004 Patch 220001
> 
> >Dependent Patchsets:
> >Issue 1658033002 Patch 310001
> >Issue 1658033002 Patch 330001
> >Issue 1658033002 Patch 350001
> 
> Using those links provided by the CL tools, you should be able to easily
figure
> out in just a few clicks that the CL for reading media track info
> (https://codereview.chromium.org/1735003004/) depended on the CL that unified
> passing media track info on the demuxer level (recently landed
> https://codereview.chromium.org/1727243002/). The CLs for passing media track
> info from FFmpeg streams into blink HTMLMediaElement
> (https://codereview.chromium.org/1735803002/) and for passing media track info
> from MSE/ChunkDemuxer into MSE SourceBuffer
> (https://codereview.chromium.org/1659653002/) both depend on the CL for
reading
> media track info (https://codereview.chromium.org/1735003004/) but don't
depend
> on each other (since MSE/ChunkDemux and FFmpeg/direct URL playback are two
> independent code paths). And the CL for introducing SourceBuffer specific
media
> track extension interfaces (https://codereview.chromium.org/1658033002/)
depends
> on the CL that implements passing of the basic track data from ChunkDemuxer
into
> blink SourceBuffer (https://codereview.chromium.org/1659653002/). Does it make
> sense?

That makes sense. Sometimes folks don't get those dependencies populated
correctly into Rietveld, but it looks like you do. No worries.

philipj_slow

4 years, 9 months ago (2016-03-15 13:08:44 UTC) #18

OK, just following the "Depends on Patchset" links sounds good. I guess the only
trick is finding the last of them.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages