SPS/PPS insertion in annex-b conversion should only be done on actual AVC keyframes |
|||||||||
Issue descriptionThe fix for bug 879734 (Mp4StreamParser now trusts the bitstream converter's analysis of coded frame keyframeness, if implemented and determinable, versus the MP4 container's metadata) was flawed: the automatic SPS/PPS insertion in the coded frame's conversion to annex-b is done if the MP4 marked the frame as a keyframe, even if the AVC frame isn't a keyframe. This is suspected of causing decode errors in some malformed streams where MP4 keyframeness mismatches AVC. To fix this: If the AVC frame is actually a nonkeyframe, SPS/PPS insertion shouldn't be done as part of the conversion. If the AVC frame is actually a keyframe, insertion should be done (there's a pre-existing issue 889309 where such insertion may be redundant, tracked separately.)
,
Sep 28
+abdulsyed@ - the CL in #1, once landed and baked, will need to be merged to M-70.
,
Sep 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ab1855aeaaf9d06fbcfec90147a2e483824a0253 commit ab1855aeaaf9d06fbcfec90147a2e483824a0253 Author: Matt Wolenetz <wolenetz@chromium.org> Date: Fri Sep 28 19:23:16 2018 MSE: Use video (AVC) keyframeness for SPS/PPS injection In 1240638ead7a, AVC keyframeness analysis result in MP4 was used instead of MP4 keyframe metadata, but the conversion of the AVC frame to Annex-B (and the conditional injection of SPS/PPS) still relied upon the MP4 keyframe metadata. Since that metadata could be incorrect, decode errors could occur on some decoders due to missing SPS/PPS data for incorrectly muxed AVC keyframes. This change performs the bitstream analysis as part of frame conversion, after converting to Annex-B, but before the conditional SPS/PPS injection, and uses the analysis's keyframe result (if determined) to condition the injection instead of the MP4 keyframe metadata. BUG= 889311 Change-Id: I1ef327ef7d399d5a9da552e0da4a58b0f0b04435 Reviewed-on: https://chromium-review.googlesource.com/1250171 Commit-Queue: Dan Sanders <sandersd@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Cr-Commit-Position: refs/heads/master@{#595176} [modify] https://crrev.com/ab1855aeaaf9d06fbcfec90147a2e483824a0253/media/formats/mp4/avc.cc [modify] https://crrev.com/ab1855aeaaf9d06fbcfec90147a2e483824a0253/media/formats/mp4/avc.h [modify] https://crrev.com/ab1855aeaaf9d06fbcfec90147a2e483824a0253/media/formats/mp4/bitstream_converter.h [modify] https://crrev.com/ab1855aeaaf9d06fbcfec90147a2e483824a0253/media/formats/mp4/hevc.cc [modify] https://crrev.com/ab1855aeaaf9d06fbcfec90147a2e483824a0253/media/formats/mp4/hevc.h [modify] https://crrev.com/ab1855aeaaf9d06fbcfec90147a2e483824a0253/media/formats/mp4/mp4_stream_parser.cc
,
Oct 1
Canary data for #3 (https://uma.googleplex.com/variations?sid=b175410f6cb74b006a8c57f62d67983b) shows that the utility process crash rate is no longer spiking for MseBufferByPts. Also, per https://uma.googleplex.com/variations?sid=8a0753b8d917e1f0e497cfe111c65843 Media.PipelineStatus.* is no longer increasing decode err or CHUNK_DEMUXER_ERROR_APPEND_FAILED (though I'm not clear precisely why that chunk_demuxer error would be fixed by #3). Accordingly, I'm requesting merge to M-70 of #3.
,
Oct 1
This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 1
,
Oct 1
wrt safety for merge, there should be no conflicts. It fixes a problem introduced by 1240638ead7a, which itself fixed other problems. With #3, we now both buffer and inject SPS/PPS more precisely for otherwise conformant AVC compressed keyframes in MP4 in the Chrome MSE implementation when the MP4 metadata for the frame is incorrect. #3 reorders two operations: analysis of the frame used to happen after insertion of SPS/PPS. With #3, the insertion can be done now on more trusted information that we get from the analysis, since the analysis is done first. Complexity is low, the scope of the change is narrow, and it doesn't introduce more technical debt. Data from Canary (see #4) appears to confirm the expectation that #3 is reducing the error rates in media parsing and decode (especially hardware H264 and decrypting decode of H264), and we've no signals that #3 is doing anything but improving behavior.
,
Oct 1
(and that previous change 1240638ead7a was already merged to M70 as fc67c41a328c4564c3e39d087b1510b28b32abce since it is critical to correctly buffering and decoding AVC-in-MP4 for content providers which unfortunately all-too-commonly have incorrect keyframe metadata in the MP4 container)
,
Oct 1
Thank you so much for more context. Approving merge for M70. Branch:3538
,
Oct 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66d853c0c682a8d554f13c38ce01870ae80f19b4 Commit: 66d853c0c682a8d554f13c38ce01870ae80f19b4 Author: wolenetz@chromium.org Commiter: wolenetz@chromium.org Date: 2018-10-01 22:41:48 +0000 UTC To M70: MSE: Use video (AVC) keyframeness for SPS/PPS injection In 1240638ead7a, AVC keyframeness analysis result in MP4 was used instead of MP4 keyframe metadata, but the conversion of the AVC frame to Annex-B (and the conditional injection of SPS/PPS) still relied upon the MP4 keyframe metadata. Since that metadata could be incorrect, decode errors could occur on some decoders due to missing SPS/PPS data for incorrectly muxed AVC keyframes. This change performs the bitstream analysis as part of frame conversion, after converting to Annex-B, but before the conditional SPS/PPS injection, and uses the analysis's keyframe result (if determined) to condition the injection instead of the MP4 keyframe metadata. BUG= 889311 TBR=sandersd@chromium.org Change-Id: I1ef327ef7d399d5a9da552e0da4a58b0f0b04435 Reviewed-on: https://chromium-review.googlesource.com/1250171 Commit-Queue: Dan Sanders <sandersd@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#595176}(cherry picked from commit ab1855aeaaf9d06fbcfec90147a2e483824a0253) Reviewed-on: https://chromium-review.googlesource.com/1255884 Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#793} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Oct 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66d853c0c682a8d554f13c38ce01870ae80f19b4 commit 66d853c0c682a8d554f13c38ce01870ae80f19b4 Author: Matt Wolenetz <wolenetz@chromium.org> Date: Mon Oct 01 22:41:48 2018 To M70: MSE: Use video (AVC) keyframeness for SPS/PPS injection In 1240638ead7a, AVC keyframeness analysis result in MP4 was used instead of MP4 keyframe metadata, but the conversion of the AVC frame to Annex-B (and the conditional injection of SPS/PPS) still relied upon the MP4 keyframe metadata. Since that metadata could be incorrect, decode errors could occur on some decoders due to missing SPS/PPS data for incorrectly muxed AVC keyframes. This change performs the bitstream analysis as part of frame conversion, after converting to Annex-B, but before the conditional SPS/PPS injection, and uses the analysis's keyframe result (if determined) to condition the injection instead of the MP4 keyframe metadata. BUG= 889311 TBR=sandersd@chromium.org Change-Id: I1ef327ef7d399d5a9da552e0da4a58b0f0b04435 Reviewed-on: https://chromium-review.googlesource.com/1250171 Commit-Queue: Dan Sanders <sandersd@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#595176}(cherry picked from commit ab1855aeaaf9d06fbcfec90147a2e483824a0253) Reviewed-on: https://chromium-review.googlesource.com/1255884 Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#793} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/66d853c0c682a8d554f13c38ce01870ae80f19b4/media/formats/mp4/avc.cc [modify] https://crrev.com/66d853c0c682a8d554f13c38ce01870ae80f19b4/media/formats/mp4/avc.h [modify] https://crrev.com/66d853c0c682a8d554f13c38ce01870ae80f19b4/media/formats/mp4/bitstream_converter.h [modify] https://crrev.com/66d853c0c682a8d554f13c38ce01870ae80f19b4/media/formats/mp4/hevc.cc [modify] https://crrev.com/66d853c0c682a8d554f13c38ce01870ae80f19b4/media/formats/mp4/hevc.h [modify] https://crrev.com/66d853c0c682a8d554f13c38ce01870ae80f19b4/media/formats/mp4/mp4_stream_parser.cc
,
Oct 2
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by wolenetz@chromium.org
, Sep 28