Issue 1812543003: Allow muting/unmuting audio through media track API

servolk

Description was changed from ========== WIP: Establish mapping between blink track id and DemuxStream in ...

4 years, 9 months ago (2016-03-17 18:27:48 UTC) #1

servolk

Description was changed from ========== Implement media track state control This CL implements basic plumbing ...

4 years, 9 months ago (2016-03-17 22:50:42 UTC) #2

servolk

servolk@chromium.org changed reviewers: + wolenetz@chromium.org

4 years, 9 months ago (2016-03-17 22:50:42 UTC) #3

servolk

Description was changed from ========== Implement media track state control This CL implements muting/unmuting audio ...

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

wolenetz

On 2016/03/22 19:07:01, servolk wrote: I'll get to this CR Monday. Of note, the title ...

4 years, 9 months ago (2016-03-25 23:01:08 UTC) #6

wolenetz

Looking pretty good. I think my biggest concern is over ownership of MediaTrack* and its ...

4 years, 8 months ago (2016-03-30 00:31:32 UTC) #7

servolk

https://codereview.chromium.org/1812543003/diff/320001/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/1812543003/diff/320001/media/base/demuxer.h#newcode70 media/base/demuxer.h:70: // (e.g. the inital stream metadata has been parsed ...

4 years, 8 months ago (2016-03-30 01:13:13 UTC) #8

servolk

https://codereview.chromium.org/1812543003/diff/320001/media/renderers/renderer_impl.cc File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/1812543003/diff/320001/media/renderers/renderer_impl.cc#newcode208 media/renderers/renderer_impl.cc:208: audio_volume_ = volume; On 2016/03/30 00:31:32, wolenetz wrote: > ...

4 years, 8 months ago (2016-03-30 01:29:49 UTC) #9

wolenetz

https://codereview.chromium.org/1812543003/diff/320001/media/renderers/renderer_impl.cc File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/1812543003/diff/320001/media/renderers/renderer_impl.cc#newcode208 media/renderers/renderer_impl.cc:208: audio_volume_ = volume; On 2016/03/30 01:29:49, servolk wrote: > ...

4 years, 8 months ago (2016-03-30 20:08:11 UTC) #10

https://codereview.chromium.org/1812543003/diff/320001/media/renderers/render...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1812543003/diff/320001/media/renderers/render...
media/renderers/renderer_impl.cc:208: audio_volume_ = volume;
On 2016/03/30 01:29:49, servolk wrote:
> On 2016/03/30 00:31:32, wolenetz wrote:
> > ISTM that this could be more readable and correct if we had something like
> > "audio output selected" separate from volume for our current
> > single-audio-track-maximum logic.
> > 
> > Example: 1 audio track enabled.
> > App disables that audio track.
> > App then sets volume to 100%.
> > * What should happen vs what actually happens?
> > I think something is mismatching at *.
> 
> That's a very good point. But please keep in mind that the most important
point
> of this CL was to demonstrate how we can convey media track status changes
from
> blink all the way to media renderer, where we can handle it. Of course ideally
> in the future we want to avoid decoding audio/video at all if the
corresponding
> media track is disabled/deselected. And this CL provides a foundation for
doing
> that by delivering the media track/demux stream status notification to the
> renderers. Changing renderers to properly support disabling/enabling tracks is
> going to be a separate non-trivial CL. One issue that immediately comes to
mind
> is how can we switch between using audio-based clock and wall clock for
driving
> video renderer on the flight.

I think in short-term, while we're still decoding even the (max 1 currently)
audio track in this CL:
1) add a TODO+crbug for tracking the disabling of audio decode (and switching to
wall-clock), and
2) use a separate flag in the audio renderer path that overrides whatever the
app-provided volume is with 0% volume iff the (max 1 currently) audio track is
disabled.

(2), especially, would fix the unexpected behavior where an app could still
cause a "disabled audio track" to render at non-zero volume.

wolenetz

Please also take a look at the bot failure on your (not yet in CR) ...

4 years, 8 months ago (2016-03-30 20:12:25 UTC) #11

wolenetz

Also: https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_demuxer.h File media/filters/chunk_demuxer.h (right): https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_demuxer.h#newcode212 media/filters/chunk_demuxer.h:212: // If |track| is null the association for ...

4 years, 8 months ago (2016-03-30 20:15:55 UTC) #12

servolk

Description was changed from ========== Implement media track state control This CL implements muting/unmuting audio ...

4 years, 8 months ago (2016-03-31 23:25:35 UTC) #13

servolk

On 2016/03/30 20:08:11, wolenetz wrote: > https://codereview.chromium.org/1812543003/diff/320001/media/renderers/renderer_impl.cc > File media/renderers/renderer_impl.cc (right): > > https://codereview.chromium.org/1812543003/diff/320001/media/renderers/renderer_impl.cc#newcode208 > ...

4 years, 8 months ago (2016-03-31 23:40:13 UTC) #14

On 2016/03/30 20:08:11, wolenetz wrote:
>
https://codereview.chromium.org/1812543003/diff/320001/media/renderers/render...
> File media/renderers/renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/1812543003/diff/320001/media/renderers/render...
> media/renderers/renderer_impl.cc:208: audio_volume_ = volume;
> On 2016/03/30 01:29:49, servolk wrote:
> > On 2016/03/30 00:31:32, wolenetz wrote:
> > > ISTM that this could be more readable and correct if we had something like
> > > "audio output selected" separate from volume for our current
> > > single-audio-track-maximum logic.
> > > 
> > > Example: 1 audio track enabled.
> > > App disables that audio track.
> > > App then sets volume to 100%.
> > > * What should happen vs what actually happens?
> > > I think something is mismatching at *.
> > 
> > That's a very good point. But please keep in mind that the most important
> point
> > of this CL was to demonstrate how we can convey media track status changes
> from
> > blink all the way to media renderer, where we can handle it. Of course
ideally
> > in the future we want to avoid decoding audio/video at all if the
> corresponding
> > media track is disabled/deselected. And this CL provides a foundation for
> doing
> > that by delivering the media track/demux stream status notification to the
> > renderers. Changing renderers to properly support disabling/enabling tracks
is
> > going to be a separate non-trivial CL. One issue that immediately comes to
> mind
> > is how can we switch between using audio-based clock and wall clock for
> driving
> > video renderer on the flight.
> 
> I think in short-term, while we're still decoding even the (max 1 currently)
> audio track in this CL:
> 1) add a TODO+crbug for tracking the disabling of audio decode (and switching
to
> wall-clock), and
> 2) use a separate flag in the audio renderer path that overrides whatever the
> app-provided volume is with 0% volume iff the (max 1 currently) audio track is
> disabled.
> 
> (2), especially, would fix the unexpected behavior where an app could still
> cause a "disabled audio track" to render at non-zero volume.

Ok, done: opened a tracking bug and added TODOs in the latest patchset + added
an audio_muted_ flag in RendererImpl to ensure disabled audio is not unmuted by
setting volume.

servolk

On 2016/03/30 20:15:55, wolenetz wrote: > Also: > > https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_demuxer.h > File media/filters/chunk_demuxer.h (right): > ...

4 years, 8 months ago (2016-03-31 23:42:16 UTC) #15

servolk

On 2016/03/31 23:42:16, servolk wrote: > On 2016/03/30 20:15:55, wolenetz wrote: > > Also: > ...

4 years, 8 months ago (2016-04-06 21:01:37 UTC) #16

On 2016/03/31 23:42:16, servolk wrote:
> On 2016/03/30 20:15:55, wolenetz wrote:
> > Also:
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_de...
> > File media/filters/chunk_demuxer.h (right):
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_de...
> > media/filters/chunk_demuxer.h:212: // If |track| is null the association for
> > this |track_id| is removed.
> > On 2016/03/30 01:13:13, servolk wrote:
> > > On 2016/03/30 00:31:32, wolenetz wrote:
> > > > who owns the memory pointed to by |track|?
> > > 
> > > It doesn't matter, because we don't actually care about the MediaTrack
> > lifetime
> > > here. We don't even need it to be alive! I'm essentially just using the
> > pointer
> > > value as a unique track id here, which will allow us to establish mapping
> from
> > > blink trackId -> MediaTrack* -> DemuxerStream*. And then we pass the
> > > DemuxerStream* pointer values to media::Renderer. We never access the
> pointee
> > > objects, so they don't need to be alive. When media::Renderer receives the
> > > DemuxerStream*, it can use it as an identifier that points to some
> > DemuxerStream
> > > and media::Renderer knows about DemuxerStreams, so then the RendererImpl
can
> > > check that the given DemuxerStream* pointers are valid and alive.
> > 
> > Hmm. Using a pointer just as an ID is not clear and risks future incorrect
> > dereferencing. Please try a better way to identify uniquely the Chromium
> > MediaTrack in these (persistent) mappings that may survive beyond the life
of
> > the Chromium MediaTrack.
> 
> Well, these tracks are actually alive. I'd say it's more like using
> base::Unretained. We don't need to keep them alive for now, since we never
> access them and just use pointers as IDs, but we can change that in the
future,
> if we decide to. But atm I can't think of any reason why we would change this.

Please take another look. In the patchset #22 I have reworked how we map track
ids to demuxer streams. Now instead of 2 maps that we had in demuxer
implementations in previous patchsets, we'll create and use the first map (that
maps track ids to MediaTrack* ptrs) only for the duration of
MediaTracksUpdatedCB invocation. This allows us to ensure that a. we never have
a dangling MediaTrack* pointer b. the logic in demuxer implementations is
simpler, since now there is a map that maps track ids directly to
DemuxerStreams. And I think this simplifies the code overall. I've also added a
bit more test coverage (in particular there is now a ChunkDemuxer unit test that
verifies that even if bytestream track ids change in the second init segment we
are still maintaining the right track id to DemuxerStream* mappings (the new
test case is added in https://codereview.chromium.org/1860263002, but this CL
also adds a bit more verification to that test case).

wolenetz

On 2016/04/06 21:01:37, servolk wrote: > On 2016/03/31 23:42:16, servolk wrote: > > On 2016/03/30 ...

4 years, 8 months ago (2016-04-08 22:43:28 UTC) #17

On 2016/04/06 21:01:37, servolk wrote:
> On 2016/03/31 23:42:16, servolk wrote:
> > On 2016/03/30 20:15:55, wolenetz wrote:
> > > Also:
> > > 
> > >
> >
>
https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_de...
> > > File media/filters/chunk_demuxer.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_de...
> > > media/filters/chunk_demuxer.h:212: // If |track| is null the association
for
> > > this |track_id| is removed.
> > > On 2016/03/30 01:13:13, servolk wrote:
> > > > On 2016/03/30 00:31:32, wolenetz wrote:
> > > > > who owns the memory pointed to by |track|?
> > > > 
> > > > It doesn't matter, because we don't actually care about the MediaTrack
> > > lifetime
> > > > here. We don't even need it to be alive! I'm essentially just using the
> > > pointer
> > > > value as a unique track id here, which will allow us to establish
mapping
> > from
> > > > blink trackId -> MediaTrack* -> DemuxerStream*. And then we pass the
> > > > DemuxerStream* pointer values to media::Renderer. We never access the
> > pointee
> > > > objects, so they don't need to be alive. When media::Renderer receives
the
> > > > DemuxerStream*, it can use it as an identifier that points to some
> > > DemuxerStream
> > > > and media::Renderer knows about DemuxerStreams, so then the RendererImpl
> can
> > > > check that the given DemuxerStream* pointers are valid and alive.
> > > 
> > > Hmm. Using a pointer just as an ID is not clear and risks future incorrect
> > > dereferencing. Please try a better way to identify uniquely the Chromium
> > > MediaTrack in these (persistent) mappings that may survive beyond the life
> of
> > > the Chromium MediaTrack.
> > 
> > Well, these tracks are actually alive. I'd say it's more like using
> > base::Unretained. We don't need to keep them alive for now, since we never
> > access them and just use pointers as IDs, but we can change that in the
> future,
> > if we decide to. But atm I can't think of any reason why we would change
this.
> 
> Please take another look. In the patchset #22 I have reworked how we map track
> ids to demuxer streams. Now instead of 2 maps that we had in demuxer
> implementations in previous patchsets, we'll create and use the first map
(that
> maps track ids to MediaTrack* ptrs) only for the duration of
> MediaTracksUpdatedCB invocation. This allows us to ensure that a. we never
have
> a dangling MediaTrack* pointer b. the logic in demuxer implementations is
> simpler, since now there is a map that maps track ids directly to
> DemuxerStreams. And I think this simplifies the code overall. I've also added
a
> bit more test coverage (in particular there is now a ChunkDemuxer unit test
that
> verifies that even if bytestream track ids change in the second init segment
we
> are still maintaining the right track id to DemuxerStream* mappings (the new
> test case is added in https://codereview.chromium.org/1860263002, but this CL
> also adds a bit more verification to that test case).

Sorry, I was sick yesterday and am just getting to this now.

wolenetz

On 2016/04/06 21:01:37, servolk wrote: > On 2016/03/31 23:42:16, servolk wrote: > > On 2016/03/30 ...

4 years, 8 months ago (2016-04-08 22:56:40 UTC) #18

On 2016/04/06 21:01:37, servolk wrote:
> On 2016/03/31 23:42:16, servolk wrote:
> > On 2016/03/30 20:15:55, wolenetz wrote:
> > > Also:
> > > 
> > >
> >
>
https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_de...
> > > File media/filters/chunk_demuxer.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_de...
> > > media/filters/chunk_demuxer.h:212: // If |track| is null the association
for
> > > this |track_id| is removed.
> > > On 2016/03/30 01:13:13, servolk wrote:
> > > > On 2016/03/30 00:31:32, wolenetz wrote:
> > > > > who owns the memory pointed to by |track|?
> > > > 
> > > > It doesn't matter, because we don't actually care about the MediaTrack
> > > lifetime
> > > > here. We don't even need it to be alive! I'm essentially just using the
> > > pointer
> > > > value as a unique track id here, which will allow us to establish
mapping
> > from
> > > > blink trackId -> MediaTrack* -> DemuxerStream*. And then we pass the
> > > > DemuxerStream* pointer values to media::Renderer. We never access the
> > pointee
> > > > objects, so they don't need to be alive. When media::Renderer receives
the
> > > > DemuxerStream*, it can use it as an identifier that points to some
> > > DemuxerStream
> > > > and media::Renderer knows about DemuxerStreams, so then the RendererImpl
> can
> > > > check that the given DemuxerStream* pointers are valid and alive.
> > > 
> > > Hmm. Using a pointer just as an ID is not clear and risks future incorrect
> > > dereferencing. Please try a better way to identify uniquely the Chromium
> > > MediaTrack in these (persistent) mappings that may survive beyond the life
> of
> > > the Chromium MediaTrack.
> > 
> > Well, these tracks are actually alive. I'd say it's more like using
> > base::Unretained. We don't need to keep them alive for now, since we never
> > access them and just use pointers as IDs, but we can change that in the
> future,
> > if we decide to. But atm I can't think of any reason why we would change
this.
> 
> Please take another look. In the patchset #22 I have reworked how we map track
> ids to demuxer streams. Now instead of 2 maps that we had in demuxer
> implementations in previous patchsets, we'll create and use the first map
(that
> maps track ids to MediaTrack* ptrs) only for the duration of
> MediaTracksUpdatedCB invocation. This allows us to ensure that a. we never
have
> a dangling MediaTrack* pointer b. the logic in demuxer implementations is
> simpler, since now there is a map that maps track ids directly to
> DemuxerStreams. And I think this simplifies the code overall. I've also added
a
> bit more test coverage (in particular there is now a ChunkDemuxer unit test
that
> verifies that even if bytestream track ids change in the second init segment
we
> are still maintaining the right track id to DemuxerStream* mappings (the new
> test case is added in https://codereview.chromium.org/1860263002, but this CL
> also adds a bit more verification to that test case).

Nice :) Thank you for keeping the potential maintenance cost of someone changing
usage/tracking of that MediaTrack* later and triggering UAF.
I've l*tm'ed % nits that prereq CL (https://codereview.chromium.org/1860263002),
and will continue CR of this one now.

wolenetz

On 2016/04/08 22:56:40, wolenetz wrote: > On 2016/04/06 21:01:37, servolk wrote: > > On 2016/03/31 ...

4 years, 8 months ago (2016-04-08 22:57:19 UTC) #19

On 2016/04/08 22:56:40, wolenetz wrote:
> On 2016/04/06 21:01:37, servolk wrote:
> > On 2016/03/31 23:42:16, servolk wrote:
> > > On 2016/03/30 20:15:55, wolenetz wrote:
> > > > Also:
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_de...
> > > > File media/filters/chunk_demuxer.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1812543003/diff/320001/media/filters/chunk_de...
> > > > media/filters/chunk_demuxer.h:212: // If |track| is null the association
> for
> > > > this |track_id| is removed.
> > > > On 2016/03/30 01:13:13, servolk wrote:
> > > > > On 2016/03/30 00:31:32, wolenetz wrote:
> > > > > > who owns the memory pointed to by |track|?
> > > > > 
> > > > > It doesn't matter, because we don't actually care about the MediaTrack
> > > > lifetime
> > > > > here. We don't even need it to be alive! I'm essentially just using
the
> > > > pointer
> > > > > value as a unique track id here, which will allow us to establish
> mapping
> > > from
> > > > > blink trackId -> MediaTrack* -> DemuxerStream*. And then we pass the
> > > > > DemuxerStream* pointer values to media::Renderer. We never access the
> > > pointee
> > > > > objects, so they don't need to be alive. When media::Renderer receives
> the
> > > > > DemuxerStream*, it can use it as an identifier that points to some
> > > > DemuxerStream
> > > > > and media::Renderer knows about DemuxerStreams, so then the
RendererImpl
> > can
> > > > > check that the given DemuxerStream* pointers are valid and alive.
> > > > 
> > > > Hmm. Using a pointer just as an ID is not clear and risks future
incorrect
> > > > dereferencing. Please try a better way to identify uniquely the Chromium
> > > > MediaTrack in these (persistent) mappings that may survive beyond the
life
> > of
> > > > the Chromium MediaTrack.
> > > 
> > > Well, these tracks are actually alive. I'd say it's more like using
> > > base::Unretained. We don't need to keep them alive for now, since we never
> > > access them and just use pointers as IDs, but we can change that in the
> > future,
> > > if we decide to. But atm I can't think of any reason why we would change
> this.
> > 
> > Please take another look. In the patchset #22 I have reworked how we map
track
> > ids to demuxer streams. Now instead of 2 maps that we had in demuxer
> > implementations in previous patchsets, we'll create and use the first map
> (that
> > maps track ids to MediaTrack* ptrs) only for the duration of
> > MediaTracksUpdatedCB invocation. This allows us to ensure that a. we never
> have
> > a dangling MediaTrack* pointer b. the logic in demuxer implementations is
> > simpler, since now there is a map that maps track ids directly to
> > DemuxerStreams. And I think this simplifies the code overall. I've also
added
> a
> > bit more test coverage (in particular there is now a ChunkDemuxer unit test
> that
> > verifies that even if bytestream track ids change in the second init segment
> we
> > are still maintaining the right track id to DemuxerStream* mappings (the new
> > test case is added in https://codereview.chromium.org/1860263002, but this
CL
> > also adds a bit more verification to that test case).
> 
> Nice :) Thank you for keeping the potential maintenance cost of someone
changing
> usage/tracking of that MediaTrack* later and triggering UAF.
> I've l*tm'ed % nits that prereq CL
(https://codereview.chromium.org/1860263002),
> and will continue CR of this one now.

argh :) s/keeping the/keeping in mind and fixing/

wolenetz

partial CR comments. more will be forthcoming ASAP, though might be Monday morning as I'm ...

4 years, 8 months ago (2016-04-08 23:32:31 UTC) #20

servolk

https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h#newcode137 media/base/demuxer.h:137: // Notifies the demuxer that track ids has been ...

4 years, 8 months ago (2016-04-08 23:47:20 UTC) #21

servolk

On 2016/04/08 23:47:20, servolk wrote: > https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h > File media/base/demuxer.h (right): > > https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h#newcode137 > ...

4 years, 8 months ago (2016-04-12 17:22:58 UTC) #22

On 2016/04/08 23:47:20, servolk wrote:
> https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h
> File media/base/demuxer.h (right):
> 
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h#n...
> media/base/demuxer.h:137: // Notifies the demuxer that track ids has been
> assigned to a media tracks.
> On 2016/04/08 23:32:31, wolenetz wrote:
> > nit: blink track ids, right?
> > nits: Also, s/has/have/ and s/a media tracks/|tracks|/.
> 
> Done.
> 
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h#n...
> media/base/demuxer.h:139: const std::vector<unsigned>& track_ids) = 0;
> On 2016/04/08 23:32:31, wolenetz wrote:
> > Comment is needed to describe that |tracks|.tracks() size must be the same
as
> > |track_ids|, and that their sequences match, such that the association of
each
> > track id to a MediaTrack is well-defined here.
> 
> Done.
> 
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h
> File media/base/media_tracks.h (right):
> 
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/media_track...
> media/base/media_tracks.h:50: void set_track_to_demux_stream_map(const
> TrackToDemuxStreamMap& m);
> On 2016/04/08 23:32:31, wolenetz wrote:
> > Hmm. Is this for testing only? It seems a bit extreme.
> 
> No. This needs to be called before reporting media tracks to the clients, so
> that clients (which interact with blink) can figure out how to map
> blink-assigned track ids to media tracks to DemuxerStreams.  This is currently
> done in MediaSourceState::OnNewConfigs and FFmpegDemuxer::OnFindStreamInfoDone
> just before MediaTrackUpdatedCB is called. But now this map has the same
> lifetime as MediaTracks and therefore we are guaranteed to have no dangling
> MediaTrack* pointers.
> 
>
https://codereview.chromium.org/1812543003/diff/530001/media/blink/webmediapl...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/1812543003/diff/530001/media/blink/webmediapl...
> media/blink/webmediaplayer_impl.cc:572: DCHECK(s);
> On 2016/04/08 23:32:31, wolenetz wrote:
> > nit: I prefer CHECK here. Perhaps also DCHECK that s is an audio demuxer
> stream.
> 
> Done.
> 
>
https://codereview.chromium.org/1812543003/diff/530001/media/blink/webmediapl...
> media/blink/webmediaplayer_impl.cc:590: DCHECK(selectedVideoStream);
> On 2016/04/08 23:32:31, wolenetz wrote:
> > nit ditto CHECK. Perhaps also DCHECK that selectedVideoStream is a video
> demuxer
> > stream.
> 
> Done.

ping

wolenetz

On 2016/04/12 17:22:58, servolk wrote: > On 2016/04/08 23:47:20, servolk wrote: > > https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h > ...

4 years, 8 months ago (2016-04-12 23:09:51 UTC) #23

On 2016/04/12 17:22:58, servolk wrote:
> On 2016/04/08 23:47:20, servolk wrote:
> > https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h
> > File media/base/demuxer.h (right):
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h#n...
> > media/base/demuxer.h:137: // Notifies the demuxer that track ids has been
> > assigned to a media tracks.
> > On 2016/04/08 23:32:31, wolenetz wrote:
> > > nit: blink track ids, right?
> > > nits: Also, s/has/have/ and s/a media tracks/|tracks|/.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/demuxer.h#n...
> > media/base/demuxer.h:139: const std::vector<unsigned>& track_ids) = 0;
> > On 2016/04/08 23:32:31, wolenetz wrote:
> > > Comment is needed to describe that |tracks|.tracks() size must be the same
> as
> > > |track_ids|, and that their sequences match, such that the association of
> each
> > > track id to a MediaTrack is well-defined here.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h
> > File media/base/media_tracks.h (right):
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/media_track...
> > media/base/media_tracks.h:50: void set_track_to_demux_stream_map(const
> > TrackToDemuxStreamMap& m);
> > On 2016/04/08 23:32:31, wolenetz wrote:
> > > Hmm. Is this for testing only? It seems a bit extreme.
> > 
> > No. This needs to be called before reporting media tracks to the clients, so
> > that clients (which interact with blink) can figure out how to map
> > blink-assigned track ids to media tracks to DemuxerStreams.  This is
currently
> > done in MediaSourceState::OnNewConfigs and
FFmpegDemuxer::OnFindStreamInfoDone
> > just before MediaTrackUpdatedCB is called. But now this map has the same
> > lifetime as MediaTracks and therefore we are guaranteed to have no dangling
> > MediaTrack* pointers.
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/530001/media/blink/webmediapl...
> > File media/blink/webmediaplayer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/530001/media/blink/webmediapl...
> > media/blink/webmediaplayer_impl.cc:572: DCHECK(s);
> > On 2016/04/08 23:32:31, wolenetz wrote:
> > > nit: I prefer CHECK here. Perhaps also DCHECK that s is an audio demuxer
> > stream.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/530001/media/blink/webmediapl...
> > media/blink/webmediaplayer_impl.cc:590: DCHECK(selectedVideoStream);
> > On 2016/04/08 23:32:31, wolenetz wrote:
> > > nit ditto CHECK. Perhaps also DCHECK that selectedVideoStream is a video
> > demuxer
> > > stream.
> > 
> > Done.
> 
> ping
Sorry - will continue this CR ASAP in morning if not further this evening.

wolenetz

Looking pretty good. I'd really encourage unifying the track map maintenance into its own class, ...

4 years, 8 months ago (2016-04-14 20:43:39 UTC) #24

Looking pretty good. I'd really encourage unifying the track map maintenance
into its own class, so reduce duplication across ffmpegdemuxer and chunkdemuxer,
and to reduce fragility and consequent need for CHECKs all over the place.

+xhwang@, since it looks like this approach will also require this CL to contain
changes to mojo_renderer_impl

https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h
File media/base/media_tracks.h (right):

https://codereview.chromium.org/1812543003/diff/530001/media/base/media_track...
media/base/media_tracks.h:50: void set_track_to_demux_stream_map(const
TrackToDemuxStreamMap& m);
On 2016/04/08 23:47:20, servolk wrote:
> On 2016/04/08 23:32:31, wolenetz wrote:
> > Hmm. Is this for testing only? It seems a bit extreme.
> 
> No. This needs to be called before reporting media tracks to the clients, so
> that clients (which interact with blink) can figure out how to map
> blink-assigned track ids to media tracks to DemuxerStreams.  This is currently
> done in MediaSourceState::OnNewConfigs and FFmpegDemuxer::OnFindStreamInfoDone
> just before MediaTrackUpdatedCB is called. But now this map has the same
> lifetime as MediaTracks and therefore we are guaranteed to have no dangling
> MediaTrack* pointers.

I see. Since MediaTracks effectively owns the map, what do you think about a
refactor of this CL to make MediaTracks have more control over how the map is
maintained? e.g. make the method that constructs the map something internal to
MediaTracks. Is that feasible? It seems a bit strange to require the user of the
API to ensure that |m| is consistent with |tracks_|, for one thing.
Alternatively, and perhaps even better, would be to make TrackToDemuxStreamMap
(perhaps named differently) be the owner of MediaTracks, and use that
TrackToDemuxStreamMap object API commonly from OnNewConfigs() and
OnFindStreamInfoDone() to centralize the map maintenance ownership. WDYT?

https://codereview.chromium.org/1812543003/diff/590001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:1048: DCHECK_EQ(tracks.tracks().size(),
track_ids.size());
These two DCHECK_EQs need to be CHECKs, since if they fail, out-of-range access
could occur.

https://codereview.chromium.org/1812543003/diff/590001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1605: DCHECK_EQ(tracks.tracks().size(),
track_ids.size());
Similarly, these two DCHECK_EQs need to be CHECK_EQs, to force fail instead of
out-of-range access.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:213: DVLOG(1) << "SetVolume ignored since audio
is muted";
nit: s/muted/muted because audio stream is enabled/ and rename audio_muted_ to
audio_stream_currently_disabled_. IMHO that would make it more clear the reason
for the muted volume result.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:244: DCHECK_EQ(demuxer_stream->type(),
DemuxerStream::AUDIO);
aside: this DCHECK (and similar for video, below) satisfies my "perhaps" comment
portion on earlier patch set.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:246: audio_stream_enabled = true;
I'm not entirely clear how this could ever not be the case. Change to DCHECK +
set the flag?

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:249: // TODO(servolk): Need to find a way to
completely disable audio (i.e. stop
nit: TODO+wolenetz

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:253: if (audio_stream_enabled) {
nit: can a track be enabled while it's already enabled? if so, we're setting the
volume twice in the renderer. Only SetVolume() if the intent is to really change
the volume.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:274: // TODO(servolk): Need to find a way to
stop video decoding/rendering when
nit: move this TODO to the else clause, and please include wolenetz in the ()
portion of the TODO.

https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:2160: // TODO(servolk): Find a way to
verify that audio is really muted/unmuted.
Hmm. This could perhaps be done in a layout test that slaves the video element
to webaudio. For now (and since we're using the mute hack), please do manual
verification please at least on desktop.
For a little deeper automation that we could do now, consider using a
StrictMock<MediaLog> with associated expectations (and add some MEDIA_LOG(INFO)
to renderer_impl when the hack_muted state changes, and verify those logs are
emitted when expected in these tests.) WDYT? Using a StrictMock<MediaLog> all
over PipelineIntegrationTest is too much for this CL (and probably too much for
future maintenance), and is *not* what I'm asking :)

Once we're not using the mute hack, but indeed disabling decode, once any
asynchronously still-in-progress decode at time of pause finally completes, the
WMPI::decodedAudioBytes() stats should stop incrementing, so we could test
better there (and similarly for video).

wolenetz

wolenetz@chromium.org changed reviewers: + xhwang@chromium.org

4 years, 8 months ago (2016-04-14 20:44:40 UTC) #25

wolenetz

Also: https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_integration_test.cc File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_integration_test.cc#newcode2160 media/test/pipeline_integration_test.cc:2160: // TODO(servolk): Find a way to verify that ...

4 years, 8 months ago (2016-04-14 22:44:12 UTC) #27

servolk

https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h File media/base/media_tracks.h (right): https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h#newcode50 media/base/media_tracks.h:50: void set_track_to_demux_stream_map(const TrackToDemuxStreamMap& m); On 2016/04/14 20:43:39, wolenetz wrote: ...

4 years, 8 months ago (2016-04-15 02:23:25 UTC) #28

https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h
File media/base/media_tracks.h (right):

https://codereview.chromium.org/1812543003/diff/530001/media/base/media_track...
media/base/media_tracks.h:50: void set_track_to_demux_stream_map(const
TrackToDemuxStreamMap& m);
On 2016/04/14 20:43:39, wolenetz wrote:
> On 2016/04/08 23:47:20, servolk wrote:
> > On 2016/04/08 23:32:31, wolenetz wrote:
> > > Hmm. Is this for testing only? It seems a bit extreme.
> > 
> > No. This needs to be called before reporting media tracks to the clients, so
> > that clients (which interact with blink) can figure out how to map
> > blink-assigned track ids to media tracks to DemuxerStreams.  This is
currently
> > done in MediaSourceState::OnNewConfigs and
FFmpegDemuxer::OnFindStreamInfoDone
> > just before MediaTrackUpdatedCB is called. But now this map has the same
> > lifetime as MediaTracks and therefore we are guaranteed to have no dangling
> > MediaTrack* pointers.
> 
> I see. Since MediaTracks effectively owns the map, what do you think about a
> refactor of this CL to make MediaTracks have more control over how the map is
> maintained? e.g. make the method that constructs the map something internal to
> MediaTracks. Is that feasible? It seems a bit strange to require the user of
the
> API to ensure that |m| is consistent with |tracks_|, for one thing.
> Alternatively, and perhaps even better, would be to make TrackToDemuxStreamMap
> (perhaps named differently) be the owner of MediaTracks, and use that
> TrackToDemuxStreamMap object API commonly from OnNewConfigs() and
> OnFindStreamInfoDone() to centralize the map maintenance ownership. WDYT?

Ok, I've refactored this code a bit, to make it simpler for MediaTrack users.
Instead of exposing the TrackToDemuxStreamMap, we can hide it (and manipulations
with it) inside the MediaTracks class, and allow demuxers to use the shared
logic for building a map for translating blink track ids into DemuxerStream*
ptrs.

https://codereview.chromium.org/1812543003/diff/590001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:1048: DCHECK_EQ(tracks.tracks().size(),
track_ids.size());
On 2016/04/14 20:43:39, wolenetz wrote:
> These two DCHECK_EQs need to be CHECKs, since if they fail, out-of-range
access
> could occur.

Done.

https://codereview.chromium.org/1812543003/diff/590001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1605: DCHECK_EQ(tracks.tracks().size(),
track_ids.size());
On 2016/04/14 20:43:39, wolenetz wrote:
> Similarly, these two DCHECK_EQs need to be CHECK_EQs, to force fail instead of
> out-of-range access.

Done.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:213: DVLOG(1) << "SetVolume ignored since audio
is muted";
On 2016/04/14 20:43:39, wolenetz wrote:
> nit: s/muted/muted because audio stream is enabled/ and rename audio_muted_ to
> audio_stream_currently_disabled_. IMHO that would make it more clear the
reason
> for the muted volume result.

Done.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:244: DCHECK_EQ(demuxer_stream->type(),
DemuxerStream::AUDIO);
On 2016/04/14 20:43:39, wolenetz wrote:
> aside: this DCHECK (and similar for video, below) satisfies my "perhaps"
comment
> portion on earlier patch set.

Acknowledged.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:246: audio_stream_enabled = true;
On 2016/04/14 20:43:39, wolenetz wrote:
> I'm not entirely clear how this could ever not be the case. Change to DCHECK +
> set the flag?

Right, for now this check is always going to succeed, since we only support 1
audio stream max. I'll change to DCHECK + set for now. We can revisit this
later, when we add multi-track support.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:249: // TODO(servolk): Need to find a way to
completely disable audio (i.e. stop
On 2016/04/14 20:43:39, wolenetz wrote:
> nit: TODO+wolenetz

Done.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:253: if (audio_stream_enabled) {
On 2016/04/14 20:43:39, wolenetz wrote:
> nit: can a track be enabled while it's already enabled? if so, we're setting
the
> volume twice in the renderer. Only SetVolume() if the intent is to really
change
> the volume.

It should never happen, since blink sends this notification only if stream state
has actually changed, I'll add a DCHECK(!audio_stream_currently_disabled_) to
ensure that stream state has actually changed.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:274: // TODO(servolk): Need to find a way to
stop video decoding/rendering when
On 2016/04/14 20:43:39, wolenetz wrote:
> nit: move this TODO to the else clause, and please include wolenetz in the ()
> portion of the TODO.

Done.

https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:2160: // TODO(servolk): Find a way to
verify that audio is really muted/unmuted.
On 2016/04/14 22:44:12, wolenetz wrote:
> On 2016/04/14 20:43:39, wolenetz wrote:
> > Hmm. This could perhaps be done in a layout test that slaves the video
element
> > to webaudio. For now (and since we're using the mute hack), please do manual
> > verification please at least on desktop.
> > For a little deeper automation that we could do now, consider using a
> > StrictMock<MediaLog> with associated expectations (and add some
> MEDIA_LOG(INFO)
> > to renderer_impl when the hack_muted state changes, and verify those logs
are
> > emitted when expected in these tests.) WDYT? Using a StrictMock<MediaLog>
all
> > over PipelineIntegrationTest is too much for this CL (and probably too much
> for
> > future maintenance), and is *not* what I'm asking :)
> > 
> > Once we're not using the mute hack, but indeed disabling decode, once any
> > asynchronously still-in-progress decode at time of pause finally completes,
> the
> > WMPI::decodedAudioBytes() stats should stop incrementing, so we could test
> > better there (and similarly for video).
> 
> For now, also you can check if the set of rendered audio samples changes
> significantly by using the hashing mechanisms in this test.
> e.g. "EXPECT_HASH_EQ("<something here>", GetAudioHash());". This macro doesn't
> do anything on some pipelines (e.g. mojo), but on supported pipelines it does
> detect changes.

Yeah, in the future we should be able to verify this using audio hash, but for
now this won't work, since audio hashing ignores volume (see NullAudioSink impl,
in particular ::SetVolume there, it does nothing). And making it use volume
might break other tests.
I've done manual verification of this both on MSE (YouTube) and FFmpeg code
paths.

servolk

https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h File media/base/media_tracks.h (right): https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h#newcode50 media/base/media_tracks.h:50: void set_track_to_demux_stream_map(const TrackToDemuxStreamMap& m); On 2016/04/15 02:23:24, servolk wrote: ...

4 years, 8 months ago (2016-04-15 02:33:00 UTC) #29

https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h
File media/base/media_tracks.h (right):

https://codereview.chromium.org/1812543003/diff/530001/media/base/media_track...
media/base/media_tracks.h:50: void set_track_to_demux_stream_map(const
TrackToDemuxStreamMap& m);
On 2016/04/15 02:23:24, servolk wrote:
> On 2016/04/14 20:43:39, wolenetz wrote:
> > On 2016/04/08 23:47:20, servolk wrote:
> > > On 2016/04/08 23:32:31, wolenetz wrote:
> > > > Hmm. Is this for testing only? It seems a bit extreme.
> > > 
> > > No. This needs to be called before reporting media tracks to the clients,
so
> > > that clients (which interact with blink) can figure out how to map
> > > blink-assigned track ids to media tracks to DemuxerStreams.  This is
> currently
> > > done in MediaSourceState::OnNewConfigs and
> FFmpegDemuxer::OnFindStreamInfoDone
> > > just before MediaTrackUpdatedCB is called. But now this map has the same
> > > lifetime as MediaTracks and therefore we are guaranteed to have no
dangling
> > > MediaTrack* pointers.
> > 
> > I see. Since MediaTracks effectively owns the map, what do you think about a
> > refactor of this CL to make MediaTracks have more control over how the map
is
> > maintained? e.g. make the method that constructs the map something internal
to
> > MediaTracks. Is that feasible? It seems a bit strange to require the user of
> the
> > API to ensure that |m| is consistent with |tracks_|, for one thing.
> > Alternatively, and perhaps even better, would be to make
TrackToDemuxStreamMap
> > (perhaps named differently) be the owner of MediaTracks, and use that
> > TrackToDemuxStreamMap object API commonly from OnNewConfigs() and
> > OnFindStreamInfoDone() to centralize the map maintenance ownership. WDYT?
> 
> Ok, I've refactored this code a bit, to make it simpler for MediaTrack users.
> Instead of exposing the TrackToDemuxStreamMap, we can hide it (and
manipulations
> with it) inside the MediaTracks class, and allow demuxers to use the shared
> logic for building a map for translating blink track ids into DemuxerStream*
> ptrs.

To expand a bit on this: I think we probably don't need to introduce a separate
class, since all we really need  in the demuxers is a map from blink track id to
DemuxStream*, so  we can just build the map here in MediaTracks and pass it to
demuxers to use.

servolk

On 2016/04/15 02:33:00, servolk wrote: > https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h > File media/base/media_tracks.h (right): > > https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h#newcode50 > ...

4 years, 8 months ago (2016-04-15 02:36:18 UTC) #30

On 2016/04/15 02:33:00, servolk wrote:
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h
> File media/base/media_tracks.h (right):
> 
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/media_track...
> media/base/media_tracks.h:50: void set_track_to_demux_stream_map(const
> TrackToDemuxStreamMap& m);
> On 2016/04/15 02:23:24, servolk wrote:
> > On 2016/04/14 20:43:39, wolenetz wrote:
> > > On 2016/04/08 23:47:20, servolk wrote:
> > > > On 2016/04/08 23:32:31, wolenetz wrote:
> > > > > Hmm. Is this for testing only? It seems a bit extreme.
> > > > 
> > > > No. This needs to be called before reporting media tracks to the
clients,
> so
> > > > that clients (which interact with blink) can figure out how to map
> > > > blink-assigned track ids to media tracks to DemuxerStreams.  This is
> > currently
> > > > done in MediaSourceState::OnNewConfigs and
> > FFmpegDemuxer::OnFindStreamInfoDone
> > > > just before MediaTrackUpdatedCB is called. But now this map has the same
> > > > lifetime as MediaTracks and therefore we are guaranteed to have no
> dangling
> > > > MediaTrack* pointers.
> > > 
> > > I see. Since MediaTracks effectively owns the map, what do you think about
a
> > > refactor of this CL to make MediaTracks have more control over how the map
> is
> > > maintained? e.g. make the method that constructs the map something
internal
> to
> > > MediaTracks. Is that feasible? It seems a bit strange to require the user
of
> > the
> > > API to ensure that |m| is consistent with |tracks_|, for one thing.
> > > Alternatively, and perhaps even better, would be to make
> TrackToDemuxStreamMap
> > > (perhaps named differently) be the owner of MediaTracks, and use that
> > > TrackToDemuxStreamMap object API commonly from OnNewConfigs() and
> > > OnFindStreamInfoDone() to centralize the map maintenance ownership. WDYT?
> > 
> > Ok, I've refactored this code a bit, to make it simpler for MediaTrack
users.
> > Instead of exposing the TrackToDemuxStreamMap, we can hide it (and
> manipulations
> > with it) inside the MediaTracks class, and allow demuxers to use the shared
> > logic for building a map for translating blink track ids into DemuxerStream*
> > ptrs.
> 
> To expand a bit on this: I think we probably don't need to introduce a
separate
> class, since all we really need  in the demuxers is a map from blink track id
to
> DemuxStream*, so  we can just build the map here in MediaTracks and pass it to
> demuxers to use.

Btw, this change has no effect on mojo yet. Since I have provided default empty
implementations for the new methods in media::Renderer interface. So now
renderers can start using those notifications about media track status changes,
but it's not mandatory yet. Plumbing media track support through mojo is not
done yet.

wolenetz

On 2016/04/15 02:36:18, servolk wrote: > On 2016/04/15 02:33:00, servolk wrote: > > > https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h ...

4 years, 8 months ago (2016-04-15 22:44:04 UTC) #31

On 2016/04/15 02:36:18, servolk wrote:
> On 2016/04/15 02:33:00, servolk wrote:
> >
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/media_tracks.h
> > File media/base/media_tracks.h (right):
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/530001/media/base/media_track...
> > media/base/media_tracks.h:50: void set_track_to_demux_stream_map(const
> > TrackToDemuxStreamMap& m);
> > On 2016/04/15 02:23:24, servolk wrote:
> > > On 2016/04/14 20:43:39, wolenetz wrote:
> > > > On 2016/04/08 23:47:20, servolk wrote:
> > > > > On 2016/04/08 23:32:31, wolenetz wrote:
> > > > > > Hmm. Is this for testing only? It seems a bit extreme.
> > > > > 
> > > > > No. This needs to be called before reporting media tracks to the
> clients,
> > so
> > > > > that clients (which interact with blink) can figure out how to map
> > > > > blink-assigned track ids to media tracks to DemuxerStreams.  This is
> > > currently
> > > > > done in MediaSourceState::OnNewConfigs and
> > > FFmpegDemuxer::OnFindStreamInfoDone
> > > > > just before MediaTrackUpdatedCB is called. But now this map has the
same
> > > > > lifetime as MediaTracks and therefore we are guaranteed to have no
> > dangling
> > > > > MediaTrack* pointers.
> > > > 
> > > > I see. Since MediaTracks effectively owns the map, what do you think
about
> a
> > > > refactor of this CL to make MediaTracks have more control over how the
map
> > is
> > > > maintained? e.g. make the method that constructs the map something
> internal
> > to
> > > > MediaTracks. Is that feasible? It seems a bit strange to require the
user
> of
> > > the
> > > > API to ensure that |m| is consistent with |tracks_|, for one thing.
> > > > Alternatively, and perhaps even better, would be to make
> > TrackToDemuxStreamMap
> > > > (perhaps named differently) be the owner of MediaTracks, and use that
> > > > TrackToDemuxStreamMap object API commonly from OnNewConfigs() and
> > > > OnFindStreamInfoDone() to centralize the map maintenance ownership.
WDYT?
> > > 
> > > Ok, I've refactored this code a bit, to make it simpler for MediaTrack
> users.
> > > Instead of exposing the TrackToDemuxStreamMap, we can hide it (and
> > manipulations
> > > with it) inside the MediaTracks class, and allow demuxers to use the
shared
> > > logic for building a map for translating blink track ids into
DemuxerStream*
> > > ptrs.
> > 
> > To expand a bit on this: I think we probably don't need to introduce a
> separate
> > class, since all we really need  in the demuxers is a map from blink track
id
> to
> > DemuxStream*, so  we can just build the map here in MediaTracks and pass it
to
> > demuxers to use.
> 
> Btw, this change has no effect on mojo yet. Since I have provided default
empty
> implementations for the new methods in media::Renderer interface. So now
> renderers can start using those notifications about media track status
changes,
> but it's not mandatory yet. Plumbing media track support through mojo is not
> done yet.

wr.t. mojo: I see. I'll move xhwang@ to cc then. I'll post updated CR comments
shortly. Thanks!

wolenetz

wolenetz@chromium.org changed reviewers: - xhwang@chromium.org

4 years, 8 months ago (2016-04-15 22:47:16 UTC) #32

wolenetz

Thanks for the refactor of the map manipulation into MediaTracks. How feasible might it be ...

4 years, 8 months ago (2016-04-15 22:47:18 UTC) #33

Thanks for the refactor of the map manipulation into MediaTracks. How feasible
might it be to not Add{Audio,Video}Track until you actually have a
DemuxerStream* and do it all at once (eliminating the need for the
SetDemuxerStreamForTrack() in MediaTracks()? My suspicion is that for
ffmpegdemuxer, that would be easy, but for MSE, more difficult since the
mediatracks are created in the stream parsers and the demuxer stream is created
in the onnewconfigs() callback execution to the create_demuxer_stream_cb_. If
you agree and don't see a simplifying refactor that fits this CL, that's fine :)

Only a couple non-nits left:

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:246: audio_stream_enabled = true;
On 2016/04/15 02:23:25, servolk wrote:
> On 2016/04/14 20:43:39, wolenetz wrote:
> > I'm not entirely clear how this could ever not be the case. Change to DCHECK
+
> > set the flag?
> 
> Right, for now this check is always going to succeed, since we only support 1
> audio stream max. I'll change to DCHECK + set for now. We can revisit this
> later, when we add multi-track support.

Ack, though it's still not DCHECK + set. Please update.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:253: if (audio_stream_enabled) {
On 2016/04/15 02:23:25, servolk wrote:
> On 2016/04/14 20:43:39, wolenetz wrote:
> > nit: can a track be enabled while it's already enabled? if so, we're setting
> the
> > volume twice in the renderer. Only SetVolume() if the intent is to really
> change
> > the volume.
> 
> It should never happen, since blink sends this notification only if stream
state
> has actually changed, I'll add a DCHECK(!audio_stream_currently_disabled_) to
> ensure that stream state has actually changed.

Ack, though now (patch set 35) only one of the two cases is DCHECKed. The other
case (audio_stream_enabled && !audio_stream_currently_disabled_) could occur and
not hit a DCHECK (though it would at least now not trigger
audio_renderer_->SetVolume(...) here anymore.

https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:2160: // TODO(servolk): Find a way to
verify that audio is really muted/unmuted.
On 2016/04/15 02:23:25, servolk wrote:
> On 2016/04/14 22:44:12, wolenetz wrote:
> > On 2016/04/14 20:43:39, wolenetz wrote:
> > > Hmm. This could perhaps be done in a layout test that slaves the video
> element
> > > to webaudio. For now (and since we're using the mute hack), please do
manual
> > > verification please at least on desktop.
> > > For a little deeper automation that we could do now, consider using a
> > > StrictMock<MediaLog> with associated expectations (and add some
> > MEDIA_LOG(INFO)
> > > to renderer_impl when the hack_muted state changes, and verify those logs
> are
> > > emitted when expected in these tests.) WDYT? Using a StrictMock<MediaLog>
> all
> > > over PipelineIntegrationTest is too much for this CL (and probably too
much
> > for
> > > future maintenance), and is *not* what I'm asking :)
> > > 
> > > Once we're not using the mute hack, but indeed disabling decode, once any
> > > asynchronously still-in-progress decode at time of pause finally
completes,
> > the
> > > WMPI::decodedAudioBytes() stats should stop incrementing, so we could test
> > > better there (and similarly for video).
> > 
> > For now, also you can check if the set of rendered audio samples changes
> > significantly by using the hashing mechanisms in this test.
> > e.g. "EXPECT_HASH_EQ("<something here>", GetAudioHash());". This macro
doesn't
> > do anything on some pipelines (e.g. mojo), but on supported pipelines it
does
> > detect changes.
> 
> Yeah, in the future we should be able to verify this using audio hash, but for
> now this won't work, since audio hashing ignores volume (see NullAudioSink
impl,
> in particular ::SetVolume there, it does nothing). And making it use volume
> might break other tests.
> I've done manual verification of this both on MSE (YouTube) and FFmpeg code
> paths.

Acknowledged.

https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
File media/base/media_tracks.cc (right):

https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
media/base/media_tracks.cc:49: track_to_demux_stream_map_[track] = stream;
Please add a CHECK that |tracks_| contains |track|. Otherwise, bad things easily
done by caller could go uncaught here. See also comment, below.

https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
media/base/media_tracks.cc:61: DCHECK(it != track_to_demux_stream_map_.end());
If SetDemuxerStreamForMediaTrack() hasn't been called for this particular track,
but maybe someone mistakenly sent some other rogue pointer to
SetDemuxerStreamForMediaTrack(), then in a release build, it->second, below,
could be bad. Pleas es/DCHECK/CHECK/ here, and also protect (also in release
builds) in SetDemuxerStreamForMediaTrack() that caller provides a valid |track|
that's in our tracks() already.

https://codereview.chromium.org/1812543003/diff/670001/media/base/media_tracks.h
File media/base/media_tracks.h (right):

https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
media/base/media_tracks.h:51: // the media |tracks_|. The size and ordering of
|track_ids| must match the
nit s/collection./collection, and |track_to_demux_stream_map_| must have an
entry for each track in |tracks_| (set by SetDemuxerStreamForMediaTrack())./

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
media/filters/media_source_state.cc:483: media_tracks_ = std::move(tracks);
minor nit: might we not want to replace our cache until we confirm success later
in this method?

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
media/filters/media_source_state.cc:675:
media_tracks_->SetDemuxerStreamForMediaTrack(audio_track, audio_);
I think this will now hit DCHECK on repeated init segment for same track (which
can occur).

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
media/filters/media_source_state.cc:679:
media_tracks_->SetDemuxerStreamForMediaTrack(video_track, video_);
ditto.

servolk

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/renderer_impl.cc File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/1812543003/diff/590001/media/renderers/renderer_impl.cc#newcode246 media/renderers/renderer_impl.cc:246: audio_stream_enabled = true; On 2016/04/15 22:47:18, wolenetz wrote: > ...

4 years, 8 months ago (2016-04-15 23:26:31 UTC) #34

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:246: audio_stream_enabled = true;
On 2016/04/15 22:47:18, wolenetz wrote:
> On 2016/04/15 02:23:25, servolk wrote:
> > On 2016/04/14 20:43:39, wolenetz wrote:
> > > I'm not entirely clear how this could ever not be the case. Change to
DCHECK
> +
> > > set the flag?
> > 
> > Right, for now this check is always going to succeed, since we only support
1
> > audio stream max. I'll change to DCHECK + set for now. We can revisit this
> > later, when we add multi-track support.
> 
> Ack, though it's still not DCHECK + set. Please update.

Done.

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:253: if (audio_stream_enabled) {
On 2016/04/15 22:47:18, wolenetz wrote:
> On 2016/04/15 02:23:25, servolk wrote:
> > On 2016/04/14 20:43:39, wolenetz wrote:
> > > nit: can a track be enabled while it's already enabled? if so, we're
setting
> > the
> > > volume twice in the renderer. Only SetVolume() if the intent is to really
> > change
> > > the volume.
> > 
> > It should never happen, since blink sends this notification only if stream
> state
> > has actually changed, I'll add a DCHECK(!audio_stream_currently_disabled_)
to
> > ensure that stream state has actually changed.
> 
> Ack, though now (patch set 35) only one of the two cases is DCHECKed. The
other
> case (audio_stream_enabled && !audio_stream_currently_disabled_) could occur
and
> not hit a DCHECK (though it would at least now not trigger
> audio_renderer_->SetVolume(...) here anymore.

Yes, I believe this is the best course of action for now, let me explain. I
actually tried to have DCHECK on both if branches (see patchset #34), but it
caused some test failures. The problem is that currently we initialize
audio_stream_currently_disabled_ to be false, but audio tracks are initially
created in disabled state and then the first audio track is explicitly enabled
by blink a bit later. So if we initially set audio_stream_currently_disabled_ to
false, then later blink enables the first audio track, we get here and
DCHECK(audio_stream_currently_disabled_) fails. Now we have two choices for how
to fix this:
1. We could initialize audio_stream_currently_disabled_ to be true and then
DCHECK and set it to false here. But I was worried that it could potentially
cause regressions, since we ignore SetVolume requests when
audio_stream_currently_disabled_ is true, and the timing of
SetVolume/OnEnabledAudioTracksChanged is not guaranteed when playback starts.
Plus unit tests don't take volume into account when computing audio hashes (see
my comments in the unit test files in this CL). So by initializing
audio_stream_currently_disabled_ to true it would be easy to accidentally
regress something and unit tests would not catch it.
2. Another solution is to have this code, where we only do SetVolume when going
from 'audio disabled' to 'audio enabled' state. By doing this we can still
initialize audio_stream_currently_disabled_ to false and thus guarantee that
SetVolume works exactly the same as before this CL, excluding the cases when
audio track has been explicitly enabled/disabled by the user. Then when playback
is being started we'll skip the unnecessary SetVolume when blink enables the
first audio track, and still allow it to happen when the audio track is
disabled/enabled.
Since changing audio_renderer volume is a temporary solution anyway, I've
decided to go with option #2 as it minimizes the chances of regressing
functionality, and we'll rework it soon enough anyway.

https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
File media/base/media_tracks.cc (right):

https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
media/base/media_tracks.cc:49: track_to_demux_stream_map_[track] = stream;
On 2016/04/15 22:47:18, wolenetz wrote:
> Please add a CHECK that |tracks_| contains |track|. Otherwise, bad things
easily
> done by caller could go uncaught here. See also comment, below.

Done.

https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
media/base/media_tracks.cc:61: DCHECK(it != track_to_demux_stream_map_.end());
On 2016/04/15 22:47:18, wolenetz wrote:
> If SetDemuxerStreamForMediaTrack() hasn't been called for this particular
track,
> but maybe someone mistakenly sent some other rogue pointer to
> SetDemuxerStreamForMediaTrack(), then in a release build, it->second, below,
> could be bad. Pleas es/DCHECK/CHECK/ here, and also protect (also in release
> builds) in SetDemuxerStreamForMediaTrack() that caller provides a valid
|track|
> that's in our tracks() already.

Done.

https://codereview.chromium.org/1812543003/diff/670001/media/base/media_tracks.h
File media/base/media_tracks.h (right):

https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
media/base/media_tracks.h:51: // the media |tracks_|. The size and ordering of
|track_ids| must match the
On 2016/04/15 22:47:18, wolenetz wrote:
> nit s/collection./collection, and |track_to_demux_stream_map_| must have an
> entry for each track in |tracks_| (set by SetDemuxerStreamForMediaTrack())./

Done.

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
media/filters/media_source_state.cc:483: media_tracks_ = std::move(tracks);
On 2016/04/15 22:47:18, wolenetz wrote:
> minor nit: might we not want to replace our cache until we confirm success
later
> in this method?

In fact it turns out that we don't need to store the MediaTracks in
MediaSourceState at all for now, so I've removed |media_tracks_| in MSS
completely.

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
media/filters/media_source_state.cc:675:
media_tracks_->SetDemuxerStreamForMediaTrack(audio_track, audio_);
On 2016/04/15 22:47:18, wolenetz wrote:
> I think this will now hit DCHECK on repeated init segment for same track
(which
> can occur).

No, it's ok, because for each new init segment a new MediaTracks object is
generated by the stream parser. So we'll set the MediaTrack* -> DemuxerStream*
associations here for the new MediaTracks and will later use it to overwrite the
current track_id -> DemuxerStream* mappings in ChunkDemuxer. So it's fine.

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
media/filters/media_source_state.cc:679:
media_tracks_->SetDemuxerStreamForMediaTrack(video_track, video_);
On 2016/04/15 22:47:18, wolenetz wrote:
> ditto.

Acknowledged.

servolk

On 2016/04/15 22:47:18, wolenetz wrote: > Thanks for the refactor of the map manipulation into ...

4 years, 8 months ago (2016-04-15 23:32:27 UTC) #35

On 2016/04/15 22:47:18, wolenetz wrote:
> Thanks for the refactor of the map manipulation into MediaTracks. How feasible
> might it be to not Add{Audio,Video}Track until you actually have a
> DemuxerStream* and do it all at once (eliminating the need for the
> SetDemuxerStreamForTrack() in MediaTracks()? My suspicion is that for
> ffmpegdemuxer, that would be easy, but for MSE, more difficult since the
> mediatracks are created in the stream parsers and the demuxer stream is
created
> in the onnewconfigs() callback execution to the create_demuxer_stream_cb_. If
> you agree and don't see a simplifying refactor that fits this CL, that's fine
:)
> 
> Only a couple non-nits left:
> 
>
https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
> File media/renderers/renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
> media/renderers/renderer_impl.cc:246: audio_stream_enabled = true;
> On 2016/04/15 02:23:25, servolk wrote:
> > On 2016/04/14 20:43:39, wolenetz wrote:
> > > I'm not entirely clear how this could ever not be the case. Change to
DCHECK
> +
> > > set the flag?
> > 
> > Right, for now this check is always going to succeed, since we only support
1
> > audio stream max. I'll change to DCHECK + set for now. We can revisit this
> > later, when we add multi-track support.
> 
> Ack, though it's still not DCHECK + set. Please update.
> 
>
https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
> media/renderers/renderer_impl.cc:253: if (audio_stream_enabled) {
> On 2016/04/15 02:23:25, servolk wrote:
> > On 2016/04/14 20:43:39, wolenetz wrote:
> > > nit: can a track be enabled while it's already enabled? if so, we're
setting
> > the
> > > volume twice in the renderer. Only SetVolume() if the intent is to really
> > change
> > > the volume.
> > 
> > It should never happen, since blink sends this notification only if stream
> state
> > has actually changed, I'll add a DCHECK(!audio_stream_currently_disabled_)
to
> > ensure that stream state has actually changed.
> 
> Ack, though now (patch set 35) only one of the two cases is DCHECKed. The
other
> case (audio_stream_enabled && !audio_stream_currently_disabled_) could occur
and
> not hit a DCHECK (though it would at least now not trigger
> audio_renderer_->SetVolume(...) here anymore.
> 
>
https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_in...
> File media/test/pipeline_integration_test.cc (right):
> 
>
https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_in...
> media/test/pipeline_integration_test.cc:2160: // TODO(servolk): Find a way to
> verify that audio is really muted/unmuted.
> On 2016/04/15 02:23:25, servolk wrote:
> > On 2016/04/14 22:44:12, wolenetz wrote:
> > > On 2016/04/14 20:43:39, wolenetz wrote:
> > > > Hmm. This could perhaps be done in a layout test that slaves the video
> > element
> > > > to webaudio. For now (and since we're using the mute hack), please do
> manual
> > > > verification please at least on desktop.
> > > > For a little deeper automation that we could do now, consider using a
> > > > StrictMock<MediaLog> with associated expectations (and add some
> > > MEDIA_LOG(INFO)
> > > > to renderer_impl when the hack_muted state changes, and verify those
logs
> > are
> > > > emitted when expected in these tests.) WDYT? Using a
StrictMock<MediaLog>
> > all
> > > > over PipelineIntegrationTest is too much for this CL (and probably too
> much
> > > for
> > > > future maintenance), and is *not* what I'm asking :)
> > > > 
> > > > Once we're not using the mute hack, but indeed disabling decode, once
any
> > > > asynchronously still-in-progress decode at time of pause finally
> completes,
> > > the
> > > > WMPI::decodedAudioBytes() stats should stop incrementing, so we could
test
> > > > better there (and similarly for video).
> > > 
> > > For now, also you can check if the set of rendered audio samples changes
> > > significantly by using the hashing mechanisms in this test.
> > > e.g. "EXPECT_HASH_EQ("<something here>", GetAudioHash());". This macro
> doesn't
> > > do anything on some pipelines (e.g. mojo), but on supported pipelines it
> does
> > > detect changes.
> > 
> > Yeah, in the future we should be able to verify this using audio hash, but
for
> > now this won't work, since audio hashing ignores volume (see NullAudioSink
> impl,
> > in particular ::SetVolume there, it does nothing). And making it use volume
> > might break other tests.
> > I've done manual verification of this both on MSE (YouTube) and FFmpeg code
> > paths.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
> File media/base/media_tracks.cc (right):
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
> media/base/media_tracks.cc:49: track_to_demux_stream_map_[track] = stream;
> Please add a CHECK that |tracks_| contains |track|. Otherwise, bad things
easily
> done by caller could go uncaught here. See also comment, below.
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
> media/base/media_tracks.cc:61: DCHECK(it != track_to_demux_stream_map_.end());
> If SetDemuxerStreamForMediaTrack() hasn't been called for this particular
track,
> but maybe someone mistakenly sent some other rogue pointer to
> SetDemuxerStreamForMediaTrack(), then in a release build, it->second, below,
> could be bad. Pleas es/DCHECK/CHECK/ here, and also protect (also in release
> builds) in SetDemuxerStreamForMediaTrack() that caller provides a valid
|track|
> that's in our tracks() already.
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/base/media_tracks.h
> File media/base/media_tracks.h (right):
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
> media/base/media_tracks.h:51: // the media |tracks_|. The size and ordering of
> |track_ids| must match the
> nit s/collection./collection, and |track_to_demux_stream_map_| must have an
> entry for each track in |tracks_| (set by SetDemuxerStreamForMediaTrack())./
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> File media/filters/media_source_state.cc (right):
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> media/filters/media_source_state.cc:483: media_tracks_ = std::move(tracks);
> minor nit: might we not want to replace our cache until we confirm success
later
> in this method?
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> media/filters/media_source_state.cc:675:
> media_tracks_->SetDemuxerStreamForMediaTrack(audio_track, audio_);
> I think this will now hit DCHECK on repeated init segment for same track
(which
> can occur).
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> media/filters/media_source_state.cc:679:
> media_tracks_->SetDemuxerStreamForMediaTrack(video_track, video_);
> ditto.

Re your top-level comment: yes, due to the way MSE works, where stream parsers
create media tracks first and then report them to
MediaSourceState::OnNewConfigs, which the creates DemuxerStreams based on the
track info, the current solution is the most straightforward. But it's an
internal implementation details that has no effect on public interfaces, so we
can always change it later. The most important thing for now is that this will
provide a mechanism for delivering media track status updates from blink level
to media::Renderer. How that mechanism works under the hood can change without
affecting the media::Renderer interface.

wolenetz

On 2016/04/15 23:32:27, servolk wrote: > On 2016/04/15 22:47:18, wolenetz wrote: > > Thanks for ...

4 years, 8 months ago (2016-04-15 23:50:11 UTC) #36

On 2016/04/15 23:32:27, servolk wrote:
> On 2016/04/15 22:47:18, wolenetz wrote:
> > Thanks for the refactor of the map manipulation into MediaTracks. How
feasible
> > might it be to not Add{Audio,Video}Track until you actually have a
> > DemuxerStream* and do it all at once (eliminating the need for the
> > SetDemuxerStreamForTrack() in MediaTracks()? My suspicion is that for
> > ffmpegdemuxer, that would be easy, but for MSE, more difficult since the
> > mediatracks are created in the stream parsers and the demuxer stream is
> created
> > in the onnewconfigs() callback execution to the create_demuxer_stream_cb_.
If
> > you agree and don't see a simplifying refactor that fits this CL, that's
fine
> :)
> > 
> > Only a couple non-nits left:
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
> > File media/renderers/renderer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
> > media/renderers/renderer_impl.cc:246: audio_stream_enabled = true;
> > On 2016/04/15 02:23:25, servolk wrote:
> > > On 2016/04/14 20:43:39, wolenetz wrote:
> > > > I'm not entirely clear how this could ever not be the case. Change to
> DCHECK
> > +
> > > > set the flag?
> > > 
> > > Right, for now this check is always going to succeed, since we only
support
> 1
> > > audio stream max. I'll change to DCHECK + set for now. We can revisit this
> > > later, when we add multi-track support.
> > 
> > Ack, though it's still not DCHECK + set. Please update.
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
> > media/renderers/renderer_impl.cc:253: if (audio_stream_enabled) {
> > On 2016/04/15 02:23:25, servolk wrote:
> > > On 2016/04/14 20:43:39, wolenetz wrote:
> > > > nit: can a track be enabled while it's already enabled? if so, we're
> setting
> > > the
> > > > volume twice in the renderer. Only SetVolume() if the intent is to
really
> > > change
> > > > the volume.
> > > 
> > > It should never happen, since blink sends this notification only if stream
> > state
> > > has actually changed, I'll add a DCHECK(!audio_stream_currently_disabled_)
> to
> > > ensure that stream state has actually changed.
> > 
> > Ack, though now (patch set 35) only one of the two cases is DCHECKed. The
> other
> > case (audio_stream_enabled && !audio_stream_currently_disabled_) could occur
> and
> > not hit a DCHECK (though it would at least now not trigger
> > audio_renderer_->SetVolume(...) here anymore.
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_in...
> > File media/test/pipeline_integration_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/590001/media/test/pipeline_in...
> > media/test/pipeline_integration_test.cc:2160: // TODO(servolk): Find a way
to
> > verify that audio is really muted/unmuted.
> > On 2016/04/15 02:23:25, servolk wrote:
> > > On 2016/04/14 22:44:12, wolenetz wrote:
> > > > On 2016/04/14 20:43:39, wolenetz wrote:
> > > > > Hmm. This could perhaps be done in a layout test that slaves the video
> > > element
> > > > > to webaudio. For now (and since we're using the mute hack), please do
> > manual
> > > > > verification please at least on desktop.
> > > > > For a little deeper automation that we could do now, consider using a
> > > > > StrictMock<MediaLog> with associated expectations (and add some
> > > > MEDIA_LOG(INFO)
> > > > > to renderer_impl when the hack_muted state changes, and verify those
> logs
> > > are
> > > > > emitted when expected in these tests.) WDYT? Using a
> StrictMock<MediaLog>
> > > all
> > > > > over PipelineIntegrationTest is too much for this CL (and probably too
> > much
> > > > for
> > > > > future maintenance), and is *not* what I'm asking :)
> > > > > 
> > > > > Once we're not using the mute hack, but indeed disabling decode, once
> any
> > > > > asynchronously still-in-progress decode at time of pause finally
> > completes,
> > > > the
> > > > > WMPI::decodedAudioBytes() stats should stop incrementing, so we could
> test
> > > > > better there (and similarly for video).
> > > > 
> > > > For now, also you can check if the set of rendered audio samples changes
> > > > significantly by using the hashing mechanisms in this test.
> > > > e.g. "EXPECT_HASH_EQ("<something here>", GetAudioHash());". This macro
> > doesn't
> > > > do anything on some pipelines (e.g. mojo), but on supported pipelines it
> > does
> > > > detect changes.
> > > 
> > > Yeah, in the future we should be able to verify this using audio hash, but
> for
> > > now this won't work, since audio hashing ignores volume (see NullAudioSink
> > impl,
> > > in particular ::SetVolume there, it does nothing). And making it use
volume
> > > might break other tests.
> > > I've done manual verification of this both on MSE (YouTube) and FFmpeg
code
> > > paths.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
> > File media/base/media_tracks.cc (right):
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
> > media/base/media_tracks.cc:49: track_to_demux_stream_map_[track] = stream;
> > Please add a CHECK that |tracks_| contains |track|. Otherwise, bad things
> easily
> > done by caller could go uncaught here. See also comment, below.
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
> > media/base/media_tracks.cc:61: DCHECK(it !=
track_to_demux_stream_map_.end());
> > If SetDemuxerStreamForMediaTrack() hasn't been called for this particular
> track,
> > but maybe someone mistakenly sent some other rogue pointer to
> > SetDemuxerStreamForMediaTrack(), then in a release build, it->second, below,
> > could be bad. Pleas es/DCHECK/CHECK/ here, and also protect (also in release
> > builds) in SetDemuxerStreamForMediaTrack() that caller provides a valid
> |track|
> > that's in our tracks() already.
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/670001/media/base/media_tracks.h
> > File media/base/media_tracks.h (right):
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/670001/media/base/media_track...
> > media/base/media_tracks.h:51: // the media |tracks_|. The size and ordering
of
> > |track_ids| must match the
> > nit s/collection./collection, and |track_to_demux_stream_map_| must have an
> > entry for each track in |tracks_| (set by SetDemuxerStreamForMediaTrack())./
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> > File media/filters/media_source_state.cc (right):
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> > media/filters/media_source_state.cc:483: media_tracks_ = std::move(tracks);
> > minor nit: might we not want to replace our cache until we confirm success
> later
> > in this method?
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> > media/filters/media_source_state.cc:675:
> > media_tracks_->SetDemuxerStreamForMediaTrack(audio_track, audio_);
> > I think this will now hit DCHECK on repeated init segment for same track
> (which
> > can occur).
> > 
> >
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> > media/filters/media_source_state.cc:679:
> > media_tracks_->SetDemuxerStreamForMediaTrack(video_track, video_);
> > ditto.
> 
> Re your top-level comment: yes, due to the way MSE works, where stream parsers
> create media tracks first and then report them to
> MediaSourceState::OnNewConfigs, which the creates DemuxerStreams based on the
> track info, the current solution is the most straightforward. But it's an
> internal implementation details that has no effect on public interfaces, so we
> can always change it later. The most important thing for now is that this will
> provide a mechanism for delivering media track status updates from blink level
> to media::Renderer. How that mechanism works under the hood can change without
> affecting the media::Renderer interface.

Agreed. We could do some internal refactor later that makes a vector of
MediaTrack the OnNewConfigs parameter, and let OnNewConfigs construct the
MediaTracks with the demuxerstream later (as one option for cleaning this up).
Not necessary in this CL, though worthy of a TODO and crbug please in this CL.

wolenetz

lgtm % TODO below (crbug for mojo, et al, renderer not to use default streamschanged ...

4 years, 8 months ago (2016-04-15 23:52:17 UTC) #37

lgtm % TODO below (crbug for mojo, et al, renderer not to use default
streamschanged impls) and % TODO above (crbug for potential cleanup of
MediaTracks::Add{Audio,Video}Track to take DemuxerStream* as required param.
Thanks!

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
media/renderers/renderer_impl.cc:253: if (audio_stream_enabled) {
On 2016/04/15 23:26:31, servolk wrote:
> On 2016/04/15 22:47:18, wolenetz wrote:
> > On 2016/04/15 02:23:25, servolk wrote:
> > > On 2016/04/14 20:43:39, wolenetz wrote:
> > > > nit: can a track be enabled while it's already enabled? if so, we're
> setting
> > > the
> > > > volume twice in the renderer. Only SetVolume() if the intent is to
really
> > > change
> > > > the volume.
> > > 
> > > It should never happen, since blink sends this notification only if stream
> > state
> > > has actually changed, I'll add a DCHECK(!audio_stream_currently_disabled_)
> to
> > > ensure that stream state has actually changed.
> > 
> > Ack, though now (patch set 35) only one of the two cases is DCHECKed. The
> other
> > case (audio_stream_enabled && !audio_stream_currently_disabled_) could occur
> and
> > not hit a DCHECK (though it would at least now not trigger
> > audio_renderer_->SetVolume(...) here anymore.
> 
> Yes, I believe this is the best course of action for now, let me explain. I
> actually tried to have DCHECK on both if branches (see patchset #34), but it
> caused some test failures. The problem is that currently we initialize
> audio_stream_currently_disabled_ to be false, but audio tracks are initially
> created in disabled state and then the first audio track is explicitly enabled
> by blink a bit later. So if we initially set audio_stream_currently_disabled_
to
> false, then later blink enables the first audio track, we get here and
> DCHECK(audio_stream_currently_disabled_) fails. Now we have two choices for
how
> to fix this:
> 1. We could initialize audio_stream_currently_disabled_ to be true and then
> DCHECK and set it to false here. But I was worried that it could potentially
> cause regressions, since we ignore SetVolume requests when
> audio_stream_currently_disabled_ is true, and the timing of
> SetVolume/OnEnabledAudioTracksChanged is not guaranteed when playback starts.
> Plus unit tests don't take volume into account when computing audio hashes
(see
> my comments in the unit test files in this CL). So by initializing
> audio_stream_currently_disabled_ to true it would be easy to accidentally
> regress something and unit tests would not catch it.
> 2. Another solution is to have this code, where we only do SetVolume when
going
> from 'audio disabled' to 'audio enabled' state. By doing this we can still
> initialize audio_stream_currently_disabled_ to false and thus guarantee that
> SetVolume works exactly the same as before this CL, excluding the cases when
> audio track has been explicitly enabled/disabled by the user. Then when
playback
> is being started we'll skip the unnecessary SetVolume when blink enables the
> first audio track, and still allow it to happen when the audio track is
> disabled/enabled.
> Since changing audio_renderer volume is a temporary solution anyway, I've
> decided to go with option #2 as it minimizes the chances of regressing
> functionality, and we'll rework it soon enough anyway.

Acknowledged. That makes sense. Thanks for explaining.

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
media/filters/media_source_state.cc:483: media_tracks_ = std::move(tracks);
On 2016/04/15 23:26:31, servolk wrote:
> On 2016/04/15 22:47:18, wolenetz wrote:
> > minor nit: might we not want to replace our cache until we confirm success
> later
> > in this method?
> 
> In fact it turns out that we don't need to store the MediaTracks in
> MediaSourceState at all for now, so I've removed |media_tracks_| in MSS
> completely.

Acknowledged.

https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
media/filters/media_source_state.cc:675:
media_tracks_->SetDemuxerStreamForMediaTrack(audio_track, audio_);
On 2016/04/15 23:26:31, servolk wrote:
> On 2016/04/15 22:47:18, wolenetz wrote:
> > I think this will now hit DCHECK on repeated init segment for same track
> (which
> > can occur).
> 
> No, it's ok, because for each new init segment a new MediaTracks object is
> generated by the stream parser. So we'll set the MediaTrack* -> DemuxerStream*
> associations here for the new MediaTracks and will later use it to overwrite
the
> current track_id -> DemuxerStream* mappings in ChunkDemuxer. So it's fine.

Acknowledged.

https://codereview.chromium.org/1812543003/diff/690001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/1812543003/diff/690001/media/base/renderer.h#...
media/base/renderer.h:87: const std::vector<const DemuxerStream*>&
enabledAudioStreams) {}
Is there a bug tracking getting mojo and any other users of this default
implementation to instead do "the right thing" and actuate the effect of streams
changing? If not, please file one. Either way, please reference it in TODO here
and in OnSelectedVideoStreamChanged(), below.

servolk

On 2016/04/15 23:52:17, wolenetz wrote: > lgtm % TODO below (crbug for mojo, et al, ...

4 years, 8 months ago (2016-04-16 00:09:57 UTC) #38

On 2016/04/15 23:52:17, wolenetz wrote:
> lgtm % TODO below (crbug for mojo, et al, renderer not to use default
> streamschanged impls) and % TODO above (crbug for potential cleanup of
> MediaTracks::Add{Audio,Video}Track to take DemuxerStream* as required param.
> Thanks!
> 
>
https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
> File media/renderers/renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/1812543003/diff/590001/media/renderers/render...
> media/renderers/renderer_impl.cc:253: if (audio_stream_enabled) {
> On 2016/04/15 23:26:31, servolk wrote:
> > On 2016/04/15 22:47:18, wolenetz wrote:
> > > On 2016/04/15 02:23:25, servolk wrote:
> > > > On 2016/04/14 20:43:39, wolenetz wrote:
> > > > > nit: can a track be enabled while it's already enabled? if so, we're
> > setting
> > > > the
> > > > > volume twice in the renderer. Only SetVolume() if the intent is to
> really
> > > > change
> > > > > the volume.
> > > > 
> > > > It should never happen, since blink sends this notification only if
stream
> > > state
> > > > has actually changed, I'll add a
DCHECK(!audio_stream_currently_disabled_)
> > to
> > > > ensure that stream state has actually changed.
> > > 
> > > Ack, though now (patch set 35) only one of the two cases is DCHECKed. The
> > other
> > > case (audio_stream_enabled && !audio_stream_currently_disabled_) could
occur
> > and
> > > not hit a DCHECK (though it would at least now not trigger
> > > audio_renderer_->SetVolume(...) here anymore.
> > 
> > Yes, I believe this is the best course of action for now, let me explain. I
> > actually tried to have DCHECK on both if branches (see patchset #34), but it
> > caused some test failures. The problem is that currently we initialize
> > audio_stream_currently_disabled_ to be false, but audio tracks are initially
> > created in disabled state and then the first audio track is explicitly
enabled
> > by blink a bit later. So if we initially set
audio_stream_currently_disabled_
> to
> > false, then later blink enables the first audio track, we get here and
> > DCHECK(audio_stream_currently_disabled_) fails. Now we have two choices for
> how
> > to fix this:
> > 1. We could initialize audio_stream_currently_disabled_ to be true and then
> > DCHECK and set it to false here. But I was worried that it could potentially
> > cause regressions, since we ignore SetVolume requests when
> > audio_stream_currently_disabled_ is true, and the timing of
> > SetVolume/OnEnabledAudioTracksChanged is not guaranteed when playback
starts.
> > Plus unit tests don't take volume into account when computing audio hashes
> (see
> > my comments in the unit test files in this CL). So by initializing
> > audio_stream_currently_disabled_ to true it would be easy to accidentally
> > regress something and unit tests would not catch it.
> > 2. Another solution is to have this code, where we only do SetVolume when
> going
> > from 'audio disabled' to 'audio enabled' state. By doing this we can still
> > initialize audio_stream_currently_disabled_ to false and thus guarantee that
> > SetVolume works exactly the same as before this CL, excluding the cases when
> > audio track has been explicitly enabled/disabled by the user. Then when
> playback
> > is being started we'll skip the unnecessary SetVolume when blink enables the
> > first audio track, and still allow it to happen when the audio track is
> > disabled/enabled.
> > Since changing audio_renderer volume is a temporary solution anyway, I've
> > decided to go with option #2 as it minimizes the chances of regressing
> > functionality, and we'll rework it soon enough anyway.
> 
> Acknowledged. That makes sense. Thanks for explaining.
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> File media/filters/media_source_state.cc (right):
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> media/filters/media_source_state.cc:483: media_tracks_ = std::move(tracks);
> On 2016/04/15 23:26:31, servolk wrote:
> > On 2016/04/15 22:47:18, wolenetz wrote:
> > > minor nit: might we not want to replace our cache until we confirm success
> > later
> > > in this method?
> > 
> > In fact it turns out that we don't need to store the MediaTracks in
> > MediaSourceState at all for now, so I've removed |media_tracks_| in MSS
> > completely.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1812543003/diff/670001/media/filters/media_so...
> media/filters/media_source_state.cc:675:
> media_tracks_->SetDemuxerStreamForMediaTrack(audio_track, audio_);
> On 2016/04/15 23:26:31, servolk wrote:
> > On 2016/04/15 22:47:18, wolenetz wrote:
> > > I think this will now hit DCHECK on repeated init segment for same track
> > (which
> > > can occur).
> > 
> > No, it's ok, because for each new init segment a new MediaTracks object is
> > generated by the stream parser. So we'll set the MediaTrack* ->
DemuxerStream*
> > associations here for the new MediaTracks and will later use it to overwrite
> the
> > current track_id -> DemuxerStream* mappings in ChunkDemuxer. So it's fine.
> 
> Acknowledged.
> 
> https://codereview.chromium.org/1812543003/diff/690001/media/base/renderer.h
> File media/base/renderer.h (right):
> 
>
https://codereview.chromium.org/1812543003/diff/690001/media/base/renderer.h#...
> media/base/renderer.h:87: const std::vector<const DemuxerStream*>&
> enabledAudioStreams) {}
> Is there a bug tracking getting mojo and any other users of this default
> implementation to instead do "the right thing" and actuate the effect of
streams
> changing? If not, please file one. Either way, please reference it in TODO
here
> and in OnSelectedVideoStreamChanged(), below.

Added TODOs in patchset #37.
Thanks for the review!

servolk

The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the ...

4 years, 8 months ago (2016-04-16 00:10:08 UTC) #40

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1812543003/710001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1812543003/710001

4 years, 8 months ago (2016-04-16 00:10:47 UTC) #41

commit-bot: I haz the power

Description was changed from ========== Allow muting/unmuting audio through media track API This CL implements ...

4 years, 8 months ago (2016-04-16 02:20:20 UTC) #42

commit-bot: I haz the power

Description was changed from ========== Allow muting/unmuting audio through media track API This CL implements ...

4 years, 8 months ago (2016-04-16 02:21:30 UTC) #44

commit-bot: I haz the power

Patchset 37 (id:??) landed as https://crrev.com/06e3a9ffca065ed73c1cd9e5d72f7ca2ee44f488 Cr-Commit-Position: refs/heads/master@{#387789}

4 years, 8 months ago (2016-04-16 02:21:31 UTC) #45

perkj_chrome

A revert of this CL (patchset #37 id:710001) has been created in https://codereview.chromium.org/1894073002/ by perkj@chromium.org. ...

4 years, 8 months ago (2016-04-18 09:37:01 UTC) #46

xhwang

xhwang@chromium.org changed reviewers: + xhwang@chromium.org

4 years, 8 months ago (2016-04-18 17:49:30 UTC) #47

xhwang

I discussed with servolk@ offline. For now we will still only support one audio and ...

4 years, 8 months ago (2016-04-18 17:49:32 UTC) #48

servolk

Description was changed from ========== Allow muting/unmuting audio through media track API This CL implements ...

4 years, 8 months ago (2016-04-19 02:42:36 UTC) #49

servolk

4 years, 8 months ago (2016-04-19 17:28:10 UTC) #50

Message was sent while issue was closed.

On 2016/04/18 09:37:01, perkj_chrome wrote:
> A revert of this CL (patchset #37 id:710001) has been created in
> https://codereview.chromium.org/1894073002/ by mailto:perkj@chromium.org.
> 
> The reason for reverting is: Tentative revert since it looks like it cause
flake
> on 
> 
>
http/tests/media/media-source/mediasource-config-change-webm-av-audio-bitrate.html.

Indeed, this CL has introduced some kind of issue, so thanks perkj@ for catching
this and reverting. I've spent quite a bit of time debugging and trying to
understand what's going on here, and I believe I understand it now. It's a race
condition that might cause occasional deadlock on MSE playback startup.
Here is what's going on:
1. At the beginning of MSE playback there is usually a lot of buffering
activity, which means that we spent significant amounts of time in
ChunkDemuxer::AppendData holding ChunkDemuxer::lock_. If the data being appended
at some point exceeds the currently known stream duration, FrameProcessor
invokes ChunkDemuxer::IncreaseDurationIfNecessary ->
ChunkDemuxer::UpdateDuration -> PipelineImpl::SetDuration, which waits to
acquire PipelineImpl::lock_.
2. Now this CL introduced another thing that happens at the beginning of media
playback - PipelineImpl::OnEnabledAudioStreamsChanged and
PipelineImpl::OnSelectedVideoStreamChanged. Those methods hold
PipelineImpl::lock_ while invoking corresponding methods in RendererImpl. Then
RendererImpl methods would call demuxer_->GetStream. Which in case of
ChunkDemuxer would need to acquire ChunkDemuxer::lock_.

So now if timing is right we might get a deadlock, where ChunkDemuxer::lock_ is
acquired by ChunkDemuxer::AppendData in #1 above, but PipelineImpl::lock_ is not
yet acquired. Then #2 happens, acquires PipelineImpl::lock_ and gets stuck
waiting forever for ChunkDemuxer::lock_.
I can think of a couple of way to fix this. The safest way seems to avoid
calling demuxer_->GetStream in RendererImpl (we don't really need them and only
use the stream pointers to do some sanity checks.

But now that this CL is reverted anyway, and after discussing this with xhwang@
I think I can also look into what it would take to rework this CL using the
Demuxer config change approach. If it works out, it might be a better long term
solution. If not, I'll just fix the deadlock as described above and will re-land
this CL.

Issue 1812543003: Allow muting/unmuting audio through media track API (Closed)