Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2281843002:
Refactor stream selection for seeking 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. |
DescriptionRefactor stream selection for seeking in FFmpegDemuxer
The current logic is not taking into account that streams can be
disabled and is heavily tied to the assumption of max 1 audio/video
stream. This refactoring strives to preserve the original logic as
much as possible, but also disregards disabled streams when searching
for the preferred seeking stream, and generalizes that logic to
potentially handle multiple audio/video streams.
BUG=641451
Committed: https://crrev.com/ed5b4e1fde34046c1f5365b30ff3b4bee7fbc4b4
Cr-Commit-Position: refs/heads/master@{#414836}
Patch Set 1 #
Total comments: 4
Patch Set 2 : CR feedback #
Messages
Total messages: 25 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Description was changed from ========== Refactor stream selection for seeking in FFmpegDemuxer The current logic is not taking into account that streams can be disabled and is heavily tied to the assumption of max 1 audio/video stream. This refactoring strives to preserve the original logic as much as possible, but also disregards disabled streams when searching for the preferred seeking stream, and generalizes that logic to potentially handle multiple audio/video streams. ========== to ========== Refactor stream selection for seeking in FFmpegDemuxer The current logic is not taking into account that streams can be disabled and is heavily tied to the assumption of max 1 audio/video stream. This refactoring strives to preserve the original logic as much as possible, but also disregards disabled streams when searching for the preferred seeking stream, and generalizes that logic to potentially handle multiple audio/video streams. ==========
servolk@chromium.org changed reviewers: + dalecurtis@chromium.org
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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, want to give it a bug since this missed m54 cut? https://codereview.chromium.org/2281843002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2281843002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1481: if (video_stream && (video_stream->start_time() == kNoTimestamp || Merge into l.1477? https://codereview.chromium.org/2281843002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1505: if (video_stream) Ternary?
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 ========== Refactor stream selection for seeking in FFmpegDemuxer The current logic is not taking into account that streams can be disabled and is heavily tied to the assumption of max 1 audio/video stream. This refactoring strives to preserve the original logic as much as possible, but also disregards disabled streams when searching for the preferred seeking stream, and generalizes that logic to potentially handle multiple audio/video streams. ========== to ========== Refactor stream selection for seeking in FFmpegDemuxer The current logic is not taking into account that streams can be disabled and is heavily tied to the assumption of max 1 audio/video stream. This refactoring strives to preserve the original logic as much as possible, but also disregards disabled streams when searching for the preferred seeking stream, and generalizes that logic to potentially handle multiple audio/video streams. BUG=641451 ==========
Fixed both comments + opened crbug.com/641451 https://codereview.chromium.org/2281843002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2281843002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1481: if (video_stream && (video_stream->start_time() == kNoTimestamp || On 2016/08/26 18:21:56, DaleCurtis wrote: > Merge into l.1477? Done. https://codereview.chromium.org/2281843002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1505: if (video_stream) On 2016/08/26 18:21:56, DaleCurtis wrote: > Ternary? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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
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/2281843002/#ps20001 (title: "CR feedback")
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 ========== Refactor stream selection for seeking in FFmpegDemuxer The current logic is not taking into account that streams can be disabled and is heavily tied to the assumption of max 1 audio/video stream. This refactoring strives to preserve the original logic as much as possible, but also disregards disabled streams when searching for the preferred seeking stream, and generalizes that logic to potentially handle multiple audio/video streams. BUG=641451 ========== to ========== Refactor stream selection for seeking in FFmpegDemuxer The current logic is not taking into account that streams can be disabled and is heavily tied to the assumption of max 1 audio/video stream. This refactoring strives to preserve the original logic as much as possible, but also disregards disabled streams when searching for the preferred seeking stream, and generalizes that logic to potentially handle multiple audio/video streams. BUG=641451 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Refactor stream selection for seeking in FFmpegDemuxer The current logic is not taking into account that streams can be disabled and is heavily tied to the assumption of max 1 audio/video stream. This refactoring strives to preserve the original logic as much as possible, but also disregards disabled streams when searching for the preferred seeking stream, and generalizes that logic to potentially handle multiple audio/video streams. BUG=641451 ========== to ========== Refactor stream selection for seeking in FFmpegDemuxer The current logic is not taking into account that streams can be disabled and is heavily tied to the assumption of max 1 audio/video stream. This refactoring strives to preserve the original logic as much as possible, but also disregards disabled streams when searching for the preferred seeking stream, and generalizes that logic to potentially handle multiple audio/video streams. BUG=641451 Committed: https://crrev.com/ed5b4e1fde34046c1f5365b30ff3b4bee7fbc4b4 Cr-Commit-Position: refs/heads/master@{#414836} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ed5b4e1fde34046c1f5365b30ff3b4bee7fbc4b4 Cr-Commit-Position: refs/heads/master@{#414836} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
