Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2866473004:
media: Disable encrypted Vorbis support on Android (Closed)
Descriptionmedia: Disable encrypted Vorbis support on Android
Currently the Vorbis decoder on Android expects 4 bytes of side channel
data in the coded byte stream. This requirement is non-conformant. This
is causing decode errors and there's no easy way to work around it in Chromium.
Given Vorbis usage is very low, this CL disables encrypted Vorbis
support on Android. For clear Vorbis we can use software decoder shipped
with Chromium and it is still supported.
BUG=710924
Review-Url: https://codereview.chromium.org/2866473004
Cr-Commit-Position: refs/heads/master@{#470402}
Committed: https://chromium.googlesource.com/chromium/src/+/83cfebca143f8af8a562d177d91c933697ab3d62
Patch Set 1 #
Total comments: 2
Patch Set 2 : comments addressed #Messages
Total messages: 20 (13 generated)
The CQ bit was checked by xhwang@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.
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org
ddorwin: PTAL jrummell: FYI
LGTM with nits Also, "side channel data in the coded byte stream that is non-conformant" sounds like the data itself is not conformant. Is that the case or is it the requirement of the four bytes that is non-conformant? https://codereview.chromium.org/2866473004/diff/1/components/cdm/browser/cdm_... File components/cdm/browser/cdm_message_filter_android.cc (right): https://codereview.chromium.org/2866473004/diff/1/components/cdm/browser/cdm_... components/cdm/browser/cdm_message_filter_android.cc:48: // Vorbis is not supported due to http://crbug.com/710924 nit: Not really "due to" - more "see." nit: End in a period.
Description was changed from ========== media: Disable encrypted Vorbis support on Android Currently the Vorbis decoder on Android expects 4 bytes of side channel data in the coded byte stream that is non-conformant. This is causing decode errors and there's no easy way to work around it in Chromium. Given Vorbis usage is very low, this CL disables encrypted Vorbis support on Android. For clear Vorbis we can use software decoder shipped with Chromium and it is still supported. BUG=710924 ========== to ========== media: Disable encrypted Vorbis support on Android Currently the Vorbis decoder on Android expects 4 bytes of side channel data in the coded byte stream. This requirement is non-conformant. This is causing decode errors and there's no easy way to work around it in Chromium. Given Vorbis usage is very low, this CL disables encrypted Vorbis support on Android. For clear Vorbis we can use software decoder shipped with Chromium and it is still supported. BUG=710924 ==========
comments addressed
https://codereview.chromium.org/2866473004/diff/1/components/cdm/browser/cdm_... File components/cdm/browser/cdm_message_filter_android.cc (right): https://codereview.chromium.org/2866473004/diff/1/components/cdm/browser/cdm_... components/cdm/browser/cdm_message_filter_android.cc:48: // Vorbis is not supported due to http://crbug.com/710924 On 2017/05/09 16:45:38, ddorwin wrote: > nit: Not really "due to" - more "see." Done. > nit: End in a period. (tip from scherkus@) I never use period after the link in case people copy it as part of the URL... I changed it to "See <url> for details." so I can append the period :)
On 2017/05/09 16:45:38, ddorwin wrote: > LGTM with nits > > Also, "side channel data in the coded byte stream that is non-conformant" sounds > like the data itself is not conformant. Is that the case or is it the > requirement of the four bytes that is non-conformant? Updated CL description to make it clear that it's the requirement that is non-conformant.
The CQ bit was checked by xhwang@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 xhwang@chromium.org
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/2866473004/#ps20001 (title: "comments addressed")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494354461687720,
"parent_rev": "27ea3538fa4f74eef95f7f53834231aa6c0892c9", "commit_rev":
"83cfebca143f8af8a562d177d91c933697ab3d62"}
Message was sent while issue was closed.
Description was changed from ========== media: Disable encrypted Vorbis support on Android Currently the Vorbis decoder on Android expects 4 bytes of side channel data in the coded byte stream. This requirement is non-conformant. This is causing decode errors and there's no easy way to work around it in Chromium. Given Vorbis usage is very low, this CL disables encrypted Vorbis support on Android. For clear Vorbis we can use software decoder shipped with Chromium and it is still supported. BUG=710924 ========== to ========== media: Disable encrypted Vorbis support on Android Currently the Vorbis decoder on Android expects 4 bytes of side channel data in the coded byte stream. This requirement is non-conformant. This is causing decode errors and there's no easy way to work around it in Chromium. Given Vorbis usage is very low, this CL disables encrypted Vorbis support on Android. For clear Vorbis we can use software decoder shipped with Chromium and it is still supported. BUG=710924 Review-Url: https://codereview.chromium.org/2866473004 Cr-Commit-Position: refs/heads/master@{#470402} Committed: https://chromium.googlesource.com/chromium/src/+/83cfebca143f8af8a562d177d91c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/83cfebca143f8af8a562d177d91c... |
