Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2284923003:
Implement support for multiple tracks in FFmpegDemuxer (Closed)
|
Created:
4 years, 3 months ago by servolk Modified:
4 years, 3 months ago Reviewers:
DaleCurtis CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement support for multiple tracks in FFmpegDemuxer
Most of the changes are fairly straightforward. We need to adjust some
logic in FFmpegDemuxer::OnFindStreamInfoDone to allow handling multiple
tracks. Also need to tweak OnEnabledAudioTracksChanged and
OnSelectedVideoTrackChanged a little. And while we are at it let's get
rid of the deprecated ScopedVector and use standard vector instead.
BUG=249427
Committed: https://crrev.com/4fba1f0c2c06184687345d202f0afd08ae8dde2a
Cr-Commit-Position: refs/heads/master@{#416163}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Added webm test + fixed CR feedback #Patch Set 3 : rebase #
Messages
Total messages: 35 (19 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement support for multiple tracks in FFmpegDemuxer Most of the changes are fairly straightforward. We need to adjust some logic in FFmpegDemuxer::OnFindStreamInfoDone to allow handling multiple tracks. Also need to tweak OnEnabledAudioTracksChanged and OnSelectedVideoTrackChanged a little. And while we are at it let's get rid of the deprecated ScopedVector and use standard vector instead. BUG=249427 ========== to ========== Implement support for multiple tracks in FFmpegDemuxer Most of the changes are fairly straightforward. We need to adjust some logic in FFmpegDemuxer::OnFindStreamInfoDone to allow handling multiple tracks. Also need to tweak OnEnabledAudioTracksChanged and OnSelectedVideoTrackChanged a little. And while we are at it let's get rid of the deprecated ScopedVector and use standard vector instead. BUG=249427 ==========
servolk@chromium.org changed reviewers: + dalecurtis@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:982: for (const auto& it : streams_) { Not really an iterator any more. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1304: if (media_tracks->tracks().size() == 0) { .empty? https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1342: for (const auto& stream : streams_) { The behavior here is different. Previously it was only enabled if the selected stream has negative timestamps, this will enable the fixups if any audio stream has negative timestamps. Practically it may not matter. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1348: (strcmp(format_context->iformat->name, "ogg") == 0 && This can be moved outside the loop. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1420: for (size_t i = 0; i < streams_.size(); ++i) { I think some of these keys might be used by MediaInternals to log UMA histograms. Can you check that this change does not break that? https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1546: enabled_streams.insert(stream); Is there a more efficient insert here? This can cause poor complexity by incrementally resizing. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1546: enabled_streams.insert(stream); Is there a more efficient insert here? This can cause poor complexity by incrementally resizing. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer_unittest.cc:1264: CreateDemuxer("bbb-320x240-2video-2audio.mp4"); mp4 clips are only tested in prop codecs builds which may not be on the cq bots iirc, can you instead add a webm test?
https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:982: for (const auto& it : streams_) { On 2016/08/29 18:15:20, DaleCurtis wrote: > Not really an iterator any more. What would be a better name? 'stream'? 's'? https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1304: if (media_tracks->tracks().size() == 0) { On 2016/08/29 18:15:20, DaleCurtis wrote: > .empty? Done. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1342: for (const auto& stream : streams_) { On 2016/08/29 18:15:20, DaleCurtis wrote: > The behavior here is different. Previously it was only enabled if the selected > stream has negative timestamps, this will enable the fixups if any audio stream > has negative timestamps. Practically it may not matter. It think it might matter. If we do have a stream that requires those fix ups, that stream may not be enabled at first. But what if it gets enabled later? We may not be able to adjust timestamps on buffers already in the pipeline, so it's better to enable fix ups before we start the pipeline. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1348: (strcmp(format_context->iformat->name, "ogg") == 0 && On 2016/08/29 18:15:21, DaleCurtis wrote: > This can be moved outside the loop. Are you talking about strcmp? But it's bound to the codec_id == VORBIS check with an &&, and then codec_id check has to happen on each stream inside the loop. If we moved that strcmp outside the loop that would change the behavior, no? https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1420: for (size_t i = 0; i < streams_.size(); ++i) { On 2016/08/29 18:15:20, DaleCurtis wrote: > I think some of these keys might be used by MediaInternals to log UMA > histograms. Can you check that this change does not break that? I have found only one place where these params seem to be used/checked: https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc... But even there some of the parameters set below are unused (e.g. video width/height), so perhaps I'm missing something. For now I've tried to avoid breaking anything by keeping parameter names the same as before for the first audio/video track. Also it looks like we are actually logging most of the relevant UMA metrics in RecordAudioCodecStats/RecordVideoCodecStats, so perhaps this is unnecessary anymore? https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1546: enabled_streams.insert(stream); On 2016/08/29 18:15:20, DaleCurtis wrote: > Is there a more efficient insert here? This can cause poor complexity by > incrementally resizing. I think we don't need to be concerned about performance here for two reasons: 1. I suspect that the number of enabled tracks is going to be <= 2 in 99% of cases even after full multi-track support is implemented. 2. Note that we are inserting into an std::set here, which is should have a decent insert performance even if the number of elements is high. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer_unittest.cc:1264: CreateDemuxer("bbb-320x240-2video-2audio.mp4"); On 2016/08/29 18:15:21, DaleCurtis wrote: > mp4 clips are only tested in prop codecs builds which may not be on the cq bots > iirc, can you instead add a webm test? Sure, I'll look into adding a webm test as well.
https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:982: for (const auto& it : streams_) { On 2016/08/29 at 18:38:20, servolk wrote: > On 2016/08/29 18:15:20, DaleCurtis wrote: > > Not really an iterator any more. > > What would be a better name? 'stream'? 's'? Yeah, you renamed all the rest to streams :) https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1348: (strcmp(format_context->iformat->name, "ogg") == 0 && On 2016/08/29 at 18:38:21, servolk wrote: > On 2016/08/29 18:15:21, DaleCurtis wrote: > > This can be moved outside the loop. > > Are you talking about strcmp? But it's bound to the codec_id == VORBIS check with an &&, and then codec_id check has to happen on each stream inside the loop. If we moved that strcmp outside the loop that would change the behavior, no? Ah, sorry I forgot this isn't explicitly for ogg anymore since we found negative ts in webm too. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1420: for (size_t i = 0; i < streams_.size(); ++i) { On 2016/08/29 at 18:38:21, servolk wrote: > On 2016/08/29 18:15:20, DaleCurtis wrote: > > I think some of these keys might be used by MediaInternals to log UMA > > histograms. Can you check that this change does not break that? > > I have found only one place where these params seem to be used/checked: > https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc... > But even there some of the parameters set below are unused (e.g. video width/height), so perhaps I'm missing something. For now I've tried to avoid breaking anything by keeping parameter names the same as before for the first audio/video track. > Also it looks like we are actually logging most of the relevant UMA metrics in RecordAudioCodecStats/RecordVideoCodecStats, so perhaps this is unnecessary anymore? It's still necessary, it allows us to break out pipeline status by codec type. I think the correct solution is to emit "*_codec_name" whenever the track switches so that the uma handler has the right status. That said, do you expect codecs to change within tracks? I think that is broken in some cases, I.e. h264 -> vp9 would not work when GpuVideDecoder is not present since we release VpxVideoDecoder to select FFmpegVideoDecoder. Ditto for audio tracks switching between Opus -> Anything.
https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:982: for (const auto& it : streams_) { On 2016/08/29 18:49:29, DaleCurtis wrote: > On 2016/08/29 at 18:38:20, servolk wrote: > > On 2016/08/29 18:15:20, DaleCurtis wrote: > > > Not really an iterator any more. > > > > What would be a better name? 'stream'? 's'? > > Yeah, you renamed all the rest to streams :) Done. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1348: (strcmp(format_context->iformat->name, "ogg") == 0 && On 2016/08/29 18:49:29, DaleCurtis wrote: > On 2016/08/29 at 18:38:21, servolk wrote: > > On 2016/08/29 18:15:21, DaleCurtis wrote: > > > This can be moved outside the loop. > > > > Are you talking about strcmp? But it's bound to the codec_id == VORBIS check > with an &&, and then codec_id check has to happen on each stream inside the > loop. If we moved that strcmp outside the loop that would change the behavior, > no? > > Ah, sorry I forgot this isn't explicitly for ogg anymore since we found negative > ts in webm too. Acknowledged. https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1420: for (size_t i = 0; i < streams_.size(); ++i) { On 2016/08/29 18:49:29, DaleCurtis wrote: > On 2016/08/29 at 18:38:21, servolk wrote: > > On 2016/08/29 18:15:20, DaleCurtis wrote: > > > I think some of these keys might be used by MediaInternals to log UMA > > > histograms. Can you check that this change does not break that? > > > > I have found only one place where these params seem to be used/checked: > > > https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc... > > But even there some of the parameters set below are unused (e.g. video > width/height), so perhaps I'm missing something. For now I've tried to avoid > breaking anything by keeping parameter names the same as before for the first > audio/video track. > > Also it looks like we are actually logging most of the relevant UMA metrics in > RecordAudioCodecStats/RecordVideoCodecStats, so perhaps this is unnecessary > anymore? > > It's still necessary, it allows us to break out pipeline status by codec type. I > think the correct solution is to emit "*_codec_name" whenever the track switches > so that the uma handler has the right status. > > That said, do you expect codecs to change within tracks? I think that is broken > in some cases, I.e. h264 -> vp9 would not work when GpuVideDecoder is not > present since we release VpxVideoDecoder to select FFmpegVideoDecoder. Ditto for > audio tracks switching between Opus -> Anything. IIUC codecs will never change within tracks, but it's possible to have one video track using h264 and another one using vp9 and switching between them dynamically should work, although of course that is going to need some more changes in renderers (and even then we probably won't be able to guarantee the switching to be completely smooth). Can we move the generation of "*_codec_name" to renderers?
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1420: for (size_t i = 0; i < streams_.size(); ++i) { On 2016/08/29 at 20:25:28, servolk wrote: > On 2016/08/29 18:49:29, DaleCurtis wrote: > > On 2016/08/29 at 18:38:21, servolk wrote: > > > On 2016/08/29 18:15:20, DaleCurtis wrote: > > > > I think some of these keys might be used by MediaInternals to log UMA > > > > histograms. Can you check that this change does not break that? > > > > > > I have found only one place where these params seem to be used/checked: > > > > > https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc... > > > But even there some of the parameters set below are unused (e.g. video > > width/height), so perhaps I'm missing something. For now I've tried to avoid > > breaking anything by keeping parameter names the same as before for the first > > audio/video track. > > > Also it looks like we are actually logging most of the relevant UMA metrics in > > RecordAudioCodecStats/RecordVideoCodecStats, so perhaps this is unnecessary > > anymore? > > > > It's still necessary, it allows us to break out pipeline status by codec type. I > > think the correct solution is to emit "*_codec_name" whenever the track switches > > so that the uma handler has the right status. > > > > That said, do you expect codecs to change within tracks? I think that is broken > > in some cases, I.e. h264 -> vp9 would not work when GpuVideDecoder is not > > present since we release VpxVideoDecoder to select FFmpegVideoDecoder. Ditto for > > audio tracks switching between Opus -> Anything. > > IIUC codecs will never change within tracks, but it's possible to have one video track using h264 and another one using vp9 and switching between them dynamically should work, although of course that is going to need some more changes in renderers (and even then we probably won't be able to guarantee the switching to be completely smooth). Can we move the generation of "*_codec_name" to renderers? Yeah, that's definitely not going to work today nor even with changes to the renderers. You need to change the DecoderSelector to not destruct unused (Video|Audio)Decoders and retry the whole suite upon track change instead of just those that haven't been tried yet. Moving generation of _codec_name to the renderer seems fine to me.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1420: for (size_t i = 0; i < streams_.size(); ++i) { On 2016/08/29 20:29:07, DaleCurtis wrote: > On 2016/08/29 at 20:25:28, servolk wrote: > > On 2016/08/29 18:49:29, DaleCurtis wrote: > > > On 2016/08/29 at 18:38:21, servolk wrote: > > > > On 2016/08/29 18:15:20, DaleCurtis wrote: > > > > > I think some of these keys might be used by MediaInternals to log UMA > > > > > histograms. Can you check that this change does not break that? > > > > > > > > I have found only one place where these params seem to be used/checked: > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc... > > > > But even there some of the parameters set below are unused (e.g. video > > > width/height), so perhaps I'm missing something. For now I've tried to avoid > > > breaking anything by keeping parameter names the same as before for the > first > > > audio/video track. > > > > Also it looks like we are actually logging most of the relevant UMA > metrics in > > > RecordAudioCodecStats/RecordVideoCodecStats, so perhaps this is unnecessary > > > anymore? > > > > > > It's still necessary, it allows us to break out pipeline status by codec > type. I > > > think the correct solution is to emit "*_codec_name" whenever the track > switches > > > so that the uma handler has the right status. > > > > > > That said, do you expect codecs to change within tracks? I think that is > broken > > > in some cases, I.e. h264 -> vp9 would not work when GpuVideDecoder is not > > > present since we release VpxVideoDecoder to select FFmpegVideoDecoder. Ditto > for > > > audio tracks switching between Opus -> Anything. > > > > IIUC codecs will never change within tracks, but it's possible to have one > video track using h264 and another one using vp9 and switching between them > dynamically should work, although of course that is going to need some more > changes in renderers (and even then we probably won't be able to guarantee the > switching to be completely smooth). Can we move the generation of "*_codec_name" > to renderers? > > Yeah, that's definitely not going to work today nor even with changes to the > renderers. You need to change the DecoderSelector to not destruct unused > (Video|Audio)Decoders and retry the whole suite upon track change instead of > just those that haven't been tried yet. > > Moving generation of _codec_name to the renderer seems fine to me. There is probably no need to change DecoderSelector at all. As I've mentioned in the previous comment, codec changes cannot happen within a stream/track. Codec changes may only happen when switching tracks. But in that case we probably don't need to reuse the DecoderStream anyway, we could probably reset/reinitialize the audio/video renderer impl with the newly selected demuxer stream and allow the RendererImpls to recreate the DecoderStream with the same initial set of potential decoders. I think it would be easier than changing the DecoderSelector/DecoderStream logic and almost the same performance-wise. WDYT?
https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1420: for (size_t i = 0; i < streams_.size(); ++i) { On 2016/08/29 21:46:24, servolk wrote: > On 2016/08/29 20:29:07, DaleCurtis wrote: > > On 2016/08/29 at 20:25:28, servolk wrote: > > > On 2016/08/29 18:49:29, DaleCurtis wrote: > > > > On 2016/08/29 at 18:38:21, servolk wrote: > > > > > On 2016/08/29 18:15:20, DaleCurtis wrote: > > > > > > I think some of these keys might be used by MediaInternals to log UMA > > > > > > histograms. Can you check that this change does not break that? > > > > > > > > > > I have found only one place where these params seem to be used/checked: > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc... > > > > > But even there some of the parameters set below are unused (e.g. video > > > > width/height), so perhaps I'm missing something. For now I've tried to > avoid > > > > breaking anything by keeping parameter names the same as before for the > > first > > > > audio/video track. > > > > > Also it looks like we are actually logging most of the relevant UMA > > metrics in > > > > RecordAudioCodecStats/RecordVideoCodecStats, so perhaps this is > unnecessary > > > > anymore? > > > > > > > > It's still necessary, it allows us to break out pipeline status by codec > > type. I > > > > think the correct solution is to emit "*_codec_name" whenever the track > > switches > > > > so that the uma handler has the right status. > > > > > > > > That said, do you expect codecs to change within tracks? I think that is > > broken > > > > in some cases, I.e. h264 -> vp9 would not work when GpuVideDecoder is not > > > > present since we release VpxVideoDecoder to select FFmpegVideoDecoder. > Ditto > > for > > > > audio tracks switching between Opus -> Anything. > > > > > > IIUC codecs will never change within tracks, but it's possible to have one > > video track using h264 and another one using vp9 and switching between them > > dynamically should work, although of course that is going to need some more > > changes in renderers (and even then we probably won't be able to guarantee the > > switching to be completely smooth). Can we move the generation of > "*_codec_name" > > to renderers? > > > > Yeah, that's definitely not going to work today nor even with changes to the > > renderers. You need to change the DecoderSelector to not destruct unused > > (Video|Audio)Decoders and retry the whole suite upon track change instead of > > just those that haven't been tried yet. > > > > Moving generation of _codec_name to the renderer seems fine to me. > > There is probably no need to change DecoderSelector at all. As I've mentioned in > the previous comment, codec changes cannot happen within a stream/track. Codec > changes may only happen when switching tracks. But in that case we probably > don't need to reuse the DecoderStream anyway, we could probably > reset/reinitialize the audio/video renderer impl with the newly selected demuxer > stream and allow the RendererImpls to recreate the DecoderStream with the same > initial set of potential decoders. I think it would be easier than changing the > DecoderSelector/DecoderStream logic and almost the same performance-wise. WDYT? Also, I have just looked a bit more into how this metadata info is used. AFAICT this data is not used for UMA at all. UMA is logged in RecordAudioCodecStats/RecordVideoCodecStats for FFmpegDemuxer and in StreamParserFactory::Create for MSE playback. None of them rely on this metadata event. Looks like this is only used for the 'Player Properties' table in the chrome://media-internals/ page. So we can probably keep it as it is and it shouldn't break anything in UMA.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/29 22:02:38, servolk wrote: > https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... > File media/filters/ffmpeg_demuxer.cc (right): > > https://codereview.chromium.org/2284923003/diff/1/media/filters/ffmpeg_demuxe... > media/filters/ffmpeg_demuxer.cc:1420: for (size_t i = 0; i < streams_.size(); > ++i) { > On 2016/08/29 21:46:24, servolk wrote: > > On 2016/08/29 20:29:07, DaleCurtis wrote: > > > On 2016/08/29 at 20:25:28, servolk wrote: > > > > On 2016/08/29 18:49:29, DaleCurtis wrote: > > > > > On 2016/08/29 at 18:38:21, servolk wrote: > > > > > > On 2016/08/29 18:15:20, DaleCurtis wrote: > > > > > > > I think some of these keys might be used by MediaInternals to log > UMA > > > > > > > histograms. Can you check that this change does not break that? > > > > > > > > > > > > I have found only one place where these params seem to be > used/checked: > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc... > > > > > > But even there some of the parameters set below are unused (e.g. video > > > > > width/height), so perhaps I'm missing something. For now I've tried to > > avoid > > > > > breaking anything by keeping parameter names the same as before for the > > > first > > > > > audio/video track. > > > > > > Also it looks like we are actually logging most of the relevant UMA > > > metrics in > > > > > RecordAudioCodecStats/RecordVideoCodecStats, so perhaps this is > > unnecessary > > > > > anymore? > > > > > > > > > > It's still necessary, it allows us to break out pipeline status by codec > > > type. I > > > > > think the correct solution is to emit "*_codec_name" whenever the track > > > switches > > > > > so that the uma handler has the right status. > > > > > > > > > > That said, do you expect codecs to change within tracks? I think that is > > > broken > > > > > in some cases, I.e. h264 -> vp9 would not work when GpuVideDecoder is > not > > > > > present since we release VpxVideoDecoder to select FFmpegVideoDecoder. > > Ditto > > > for > > > > > audio tracks switching between Opus -> Anything. > > > > > > > > IIUC codecs will never change within tracks, but it's possible to have one > > > video track using h264 and another one using vp9 and switching between them > > > dynamically should work, although of course that is going to need some more > > > changes in renderers (and even then we probably won't be able to guarantee > the > > > switching to be completely smooth). Can we move the generation of > > "*_codec_name" > > > to renderers? > > > > > > Yeah, that's definitely not going to work today nor even with changes to the > > > renderers. You need to change the DecoderSelector to not destruct unused > > > (Video|Audio)Decoders and retry the whole suite upon track change instead of > > > just those that haven't been tried yet. > > > > > > Moving generation of _codec_name to the renderer seems fine to me. > > > > There is probably no need to change DecoderSelector at all. As I've mentioned > in > > the previous comment, codec changes cannot happen within a stream/track. Codec > > changes may only happen when switching tracks. But in that case we probably > > don't need to reuse the DecoderStream anyway, we could probably > > reset/reinitialize the audio/video renderer impl with the newly selected > demuxer > > stream and allow the RendererImpls to recreate the DecoderStream with the same > > initial set of potential decoders. I think it would be easier than changing > the > > DecoderSelector/DecoderStream logic and almost the same performance-wise. > WDYT? > > Also, I have just looked a bit more into how this metadata info is used. AFAICT > this data is not used for UMA at all. UMA is logged in > RecordAudioCodecStats/RecordVideoCodecStats for FFmpegDemuxer and in > StreamParserFactory::Create for MSE playback. None of them rely on this metadata > event. Looks like this is only used for the 'Player Properties' table in the > chrome://media-internals/ page. So we can probably keep it as it is and it > shouldn't break anything in UMA. Dale, given our offline discussion (this is simply enabling multiple tracks/streams support via FFmpegDemuxer, but support for switching between tracks will need further changes in renderers) are you ok with landing this?
lgtm if uma still work.
On 2016/09/01 22:25:01, DaleCurtis wrote: > lgtm if uma still work. Thanks. I've made sure that FFmpegDemuxer::LogMetadata preserves all metadata parameters that are used for UMA, so I believe UMA should still work. I'll keep an eye on it when this change rolls out in canary.
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by servolk@chromium.org
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2284923003/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement support for multiple tracks in FFmpegDemuxer Most of the changes are fairly straightforward. We need to adjust some logic in FFmpegDemuxer::OnFindStreamInfoDone to allow handling multiple tracks. Also need to tweak OnEnabledAudioTracksChanged and OnSelectedVideoTrackChanged a little. And while we are at it let's get rid of the deprecated ScopedVector and use standard vector instead. BUG=249427 ========== to ========== Implement support for multiple tracks in FFmpegDemuxer Most of the changes are fairly straightforward. We need to adjust some logic in FFmpegDemuxer::OnFindStreamInfoDone to allow handling multiple tracks. Also need to tweak OnEnabledAudioTracksChanged and OnSelectedVideoTrackChanged a little. And while we are at it let's get rid of the deprecated ScopedVector and use standard vector instead. BUG=249427 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Implement support for multiple tracks in FFmpegDemuxer Most of the changes are fairly straightforward. We need to adjust some logic in FFmpegDemuxer::OnFindStreamInfoDone to allow handling multiple tracks. Also need to tweak OnEnabledAudioTracksChanged and OnSelectedVideoTrackChanged a little. And while we are at it let's get rid of the deprecated ScopedVector and use standard vector instead. BUG=249427 ========== to ========== Implement support for multiple tracks in FFmpegDemuxer Most of the changes are fairly straightforward. We need to adjust some logic in FFmpegDemuxer::OnFindStreamInfoDone to allow handling multiple tracks. Also need to tweak OnEnabledAudioTracksChanged and OnSelectedVideoTrackChanged a little. And while we are at it let's get rid of the deprecated ScopedVector and use standard vector instead. BUG=249427 Committed: https://crrev.com/4fba1f0c2c06184687345d202f0afd08ae8dde2a Cr-Commit-Position: refs/heads/master@{#416163} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4fba1f0c2c06184687345d202f0afd08ae8dde2a Cr-Commit-Position: refs/heads/master@{#416163} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
