Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2361903002:
mp4: skip 0 size trun samples (Closed)
|
Created:
4 years, 3 months ago by chcunningham Modified:
4 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMSE mp4: skip 0 size trun samples.
Code that follows assumes samples have some non-zero size.
Failing to skip causes unsafe memory de-references and
decoder errors (observed in email thread with twitch.tv
Committed: https://crrev.com/30ed88a9083a5a37b5dfcba911ca45e5f6e00913
Cr-Commit-Position: refs/heads/master@{#420647}
Patch Set 1 #
Total comments: 3
Patch Set 2 : replace DVLOG with LIMITED_MEDIA_LOG #
Total comments: 3
Messages
Total messages: 19 (12 generated)
wolenetz@chromium.org changed reviewers: + wolenetz@chromium.org
LGTM https://codereview.chromium.org/2361903002/diff/1/media/formats/mp4/mp4_strea... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/2361903002/diff/1/media/formats/mp4/mp4_strea... media/formats/mp4/mp4_stream_parser.cc:622: &frame_buf[0], frame_buf.size(), runs_->is_keyframe(), buffer_type, Yeah, I found I couldn't get DEBUG build to get to the repro case without special-casing &frame_buf[0] here and at least one other place (in frame conversion, above, I think).
https://codereview.chromium.org/2361903002/diff/1/media/formats/mp4/mp4_strea... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/2361903002/diff/1/media/formats/mp4/mp4_strea... media/formats/mp4/mp4_stream_parser.cc:561: DVLOG(3) << " Skipping sample of size 0"; Actually - from chat, let's do a LIMITED_MEDIA_LOG here to help MSE API users. unlimited MEDIA_LOG might spam. DVLOG is useless to most MSE API users.
chcunningham@google.com changed reviewers: + chcunningham@google.com
Please CQ if it looks good https://codereview.chromium.org/2361903002/diff/1/media/formats/mp4/mp4_strea... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/2361903002/diff/1/media/formats/mp4/mp4_strea... media/formats/mp4/mp4_stream_parser.cc:561: DVLOG(3) << " Skipping sample of size 0"; On 2016/09/22 22:59:10, wolenetz wrote: > Actually - from chat, let's do a LIMITED_MEDIA_LOG here to help MSE API users. > unlimited MEDIA_LOG might spam. DVLOG is useless to most MSE API users. Done.
The CQ bit was checked by chcunningham@google.com 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 ========== mp4: skip 0 size trun samples BUG= ========== to ========== MSE mp4: skip 0 size trun samples. Code that follows assumes samples have some non-zero size. Failing to skip causes unsafe memory de-references and decoder errors (observed in email thread with twitch.tv ==========
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 chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2361903002/#ps20001 (title: "replace DVLOG with LIMITED_MEDIA_LOG")
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 ========== MSE mp4: skip 0 size trun samples. Code that follows assumes samples have some non-zero size. Failing to skip causes unsafe memory de-references and decoder errors (observed in email thread with twitch.tv ========== to ========== MSE mp4: skip 0 size trun samples. Code that follows assumes samples have some non-zero size. Failing to skip causes unsafe memory de-references and decoder errors (observed in email thread with twitch.tv ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MSE mp4: skip 0 size trun samples. Code that follows assumes samples have some non-zero size. Failing to skip causes unsafe memory de-references and decoder errors (observed in email thread with twitch.tv ========== to ========== MSE mp4: skip 0 size trun samples. Code that follows assumes samples have some non-zero size. Failing to skip causes unsafe memory de-references and decoder errors (observed in email thread with twitch.tv Committed: https://crrev.com/30ed88a9083a5a37b5dfcba911ca45e5f6e00913 Cr-Commit-Position: refs/heads/master@{#420647} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/30ed88a9083a5a37b5dfcba911ca45e5f6e00913 Cr-Commit-Position: refs/heads/master@{#420647}
Message was sent while issue was closed.
I noted a typo and a fix after you landed the latest patchset. I've uploaded https://codereview.chromium.org/2365653006 for your review, which fixes these comments: https://codereview.chromium.org/2361903002/diff/20001/media/formats/mp4/mp4_s... File media/formats/mp4/mp4_stream_parser.cc (right): https://codereview.chromium.org/2361903002/diff/20001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:38: } nit: // namespace https://codereview.chromium.org/2361903002/diff/20001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.cc:50: num_top_level_box_skipped_(0), aside: this other counter is no longer used. we should remove it. https://codereview.chromium.org/2361903002/diff/20001/media/formats/mp4/mp4_s... File media/formats/mp4/mp4_stream_parser.h (right): https://codereview.chromium.org/2361903002/diff/20001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser.h:133: int num_emtpy_samples_skipped_; nit: typo. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
