Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(49)

Issue 2254733006: Allow MP4 parser to handle multiple audio and video tracks (Closed)

Created:
4 years, 4 months ago by servolk
Modified:
4 years, 3 months ago
CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@merged-buffers-map
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Allow MP4 parser to handle multiple audio and video tracks Added a new multi-track (2 audio + 2 video tracks) .mp4 file for tests and MP4StreamParser to handle multiple tracks properly. BUG=249427, 249428 Committed: https://crrev.com/634e7ec2079af0bea0029681ebf04524ed7b2bbb Cr-Commit-Position: refs/heads/master@{#414872}

Patch Set 1 #

Patch Set 2 : nits #

Patch Set 3 : Make .mp4 file smaller #

Patch Set 4 : rebase #

Patch Set 5 : upload .mp4 #

Patch Set 6 : Clear track ids at the beginning of ParseMoov #

Total comments: 15

Patch Set 7 : CR feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+115 lines, -59 lines) Patch
M media/formats/mp4/avc.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M media/formats/mp4/mp4_stream_parser.h View 1 1 chunk +3 lines, -4 lines 0 comments Download
M media/formats/mp4/mp4_stream_parser.cc View 1 2 3 4 5 6 10 chunks +40 lines, -26 lines 0 comments Download
M media/formats/mp4/mp4_stream_parser_unittest.cc View 1 2 3 4 5 6 3 chunks +56 lines, -29 lines 0 comments Download
M media/test/data/README View 1 2 3 4 5 6 1 chunk +13 lines, -0 lines 0 comments Download
A media/test/data/bbb-320x240-2video-2audio.mp4 View 1 2 Binary file 0 comments Download

Depends on Patchset:

Messages

Total messages: 44 (35 generated)
servolk
4 years, 4 months ago (2016-08-20 01:22:13 UTC) #21
chcunningham1
lgtm https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_stream_parser.cc File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_stream_parser.cc#newcode367 media/formats/mp4/mp4_stream_parser.cc:367: is_track_encrypted_[video_track_id] = is_track_encrypted; Maybe capture the return value ...
4 years, 4 months ago (2016-08-23 01:54:32 UTC) #27
wolenetz
LGTM % prereq CL landing first, and % nits and fixing non-first A/V tracks' kinds ...
4 years, 4 months ago (2016-08-23 22:51:42 UTC) #28
servolk
https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_stream_parser.cc File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_stream_parser.cc#newcode201 media/formats/mp4/mp4_stream_parser.cc:201: // TODO(strobe): Only the first audio and video track ...
4 years, 4 months ago (2016-08-24 00:53:49 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2254733006/120001
4 years, 3 months ago (2016-08-26 17:46:20 UTC) #36
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/282270)
4 years, 3 months ago (2016-08-26 22:30:14 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2254733006/120001
4 years, 3 months ago (2016-08-26 22:33:08 UTC) #40
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 3 months ago (2016-08-27 00:18:41 UTC) #42
commit-bot: I haz the power
4 years, 3 months ago (2016-08-27 00:20:29 UTC) #44
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/634e7ec2079af0bea0029681ebf04524ed7b2bbb
Cr-Commit-Position: refs/heads/master@{#414872}

Powered by Google App Engine
This is Rietveld 408576698