|
|
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. |
DescriptionAllow 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 #
Depends on Patchset: Messages
Total messages: 44 (35 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 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: This issue passed the CQ dry run.
Description was changed from ========== 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 ========== to ========== 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 ==========
servolk@chromium.org changed reviewers: + chcunningham@chromium.org, wolenetz@chromium.org
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: This issue passed the CQ dry run.
chcunningham@google.com changed reviewers: + chcunningham@google.com
lgtm https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:367: is_track_encrypted_[video_track_id] = is_track_encrypted; Maybe capture the return value and check that its not already present in the set (i.e. confirm media does not re-use track ids)? https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:615: DVLOG(3) << "Emit " << (audio ? "audio" : "video") << " frame: " what if the track is text? will you say "video" here? https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... File media/formats/mp4/mp4_stream_parser_unittest.cc (right): https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser_unittest.cc:541: ParseMP4File("bbb-320x240-2video-2audio.mp4", 4096); whats the bbb stand for?
LGTM % prereq CL landing first, and % nits and fixing non-first A/V tracks' kinds to be "translation" IIUC from the in-band sourcing spec. Note that that spec has further specialization of "kinds" attribute for EAC3/AC3 tracks, which might be "descriptions" or "main-desc". Fixing this latter case (EAC3/AC3) can be done separately: please file a crbug to track that though. https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:201: // TODO(strobe): Only the first audio and video track present in a file are Comment is obsolete. https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:327: media_tracks->AddAudioTrack(audio_config, audio_track_id, "main", Are all audio tracks "main" in a multi-track audio mp4? IIRC, the inband sourcing spec may help answer this. Consult: https://dev.w3.org/html5/html-sourcing-inband-tracks/#mpeg4 "Track Attributes for sourced Audio and Video Tracks" section. https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:367: is_track_encrypted_[video_track_id] = is_track_encrypted; On 2016/08/23 01:54:32, chcunningham1 wrote: > Maybe capture the return value and check that its not already present in the set > (i.e. confirm media does not re-use track ids)? +1 to unique track id checking (here and in the audio case) https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:384: media_tracks->AddVideoTrack(video_config, video_track_id, "main", ditto: "main" is only for the first video track. https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:615: DVLOG(3) << "Emit " << (audio ? "audio" : "video") << " frame: " On 2016/08/23 01:54:32, chcunningham1 wrote: > what if the track is text? will you say "video" here? At the moment, Chrome MSE mp4 parser doesn't parse text tracks. https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... File media/formats/mp4/mp4_stream_parser_unittest.cc (right): https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser_unittest.cc:541: ParseMP4File("bbb-320x240-2video-2audio.mp4", 4096); On 2016/08/23 01:54:32, chcunningham1 wrote: > whats the bbb stand for? Big Buck Bunny, I presume :) See the README updated in this CL.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:201: // TODO(strobe): Only the first audio and video track present in a file are On 2016/08/23 22:51:42, wolenetz wrote: > Comment is obsolete. Done. https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:327: media_tracks->AddAudioTrack(audio_config, audio_track_id, "main", On 2016/08/23 22:51:42, wolenetz wrote: > Are all audio tracks "main" in a multi-track audio mp4? IIRC, the inband > sourcing spec may help answer this. > Consult: https://dev.w3.org/html5/html-sourcing-inband-tracks/#mpeg4 "Track > Attributes for sourced Audio and Video Tracks" section. > Done. https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:367: is_track_encrypted_[video_track_id] = is_track_encrypted; On 2016/08/23 22:51:42, wolenetz wrote: > On 2016/08/23 01:54:32, chcunningham1 wrote: > > Maybe capture the return value and check that its not already present in the > set > > (i.e. confirm media does not re-use track ids)? > > +1 to unique track id checking (here and in the audio case) Done. https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:384: media_tracks->AddVideoTrack(video_config, video_track_id, "main", On 2016/08/23 22:51:42, wolenetz wrote: > ditto: "main" is only for the first video track. Done. https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser.cc:615: DVLOG(3) << "Emit " << (audio ? "audio" : "video") << " frame: " On 2016/08/23 22:51:42, wolenetz wrote: > On 2016/08/23 01:54:32, chcunningham1 wrote: > > what if the track is text? will you say "video" here? > > At the moment, Chrome MSE mp4 parser doesn't parse text tracks. Yup, text tracks in .mp4 are not supported for now. https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... File media/formats/mp4/mp4_stream_parser_unittest.cc (right): https://codereview.chromium.org/2254733006/diff/100001/media/formats/mp4/mp4_... media/formats/mp4/mp4_stream_parser_unittest.cc:541: ParseMP4File("bbb-320x240-2video-2audio.mp4", 4096); On 2016/08/23 22:51:42, wolenetz wrote: > On 2016/08/23 01:54:32, chcunningham1 wrote: > > whats the bbb stand for? > > Big Buck Bunny, I presume :) See the README updated in this CL. Yep, it's Big Buck Bunny. Btw, I just took a look at https://peach.blender.org/about/, looks like we'll also need to add an attribution to the README file, that would look like this: (c) copyright 2008, Blender Foundation / www.bigbuckbunny.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.
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@google.com, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2254733006/#ps120001 (title: "CR feedback")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/634e7ec2079af0bea0029681ebf04524ed7b2bbb Cr-Commit-Position: refs/heads/master@{#414872} |