|
|
Created:
3 years, 7 months ago by yucliu1 Modified:
3 years, 7 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, eme-reviews_chromium.org, halliwell+watch_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd EME contentType checks for mp2t
We don't have 'mp2t/audio' for audio streams. For video streams, stream
parser supports encrypted AVC stream. The patch adds the supporting for
mp2t avc.
BUG=708261
TEST=Test on chromecast and Android.
Review-Url: https://codereview.chromium.org/2881443002
Cr-Commit-Position: refs/heads/master@{#473633}
Committed: https://chromium.googlesource.com/chromium/src/+/a26af12f05559a607d50c22d94135b18902167eb
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebase #Patch Set 3 : Rename and VP9 #Patch Set 4 : Restore PS2 #Patch Set 5 : Name and comment #Messages
Total messages: 39 (15 generated)
Description was changed from ========== Add EME contentType checks for mp2t We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. The patch adds the supporting for mp2t avc. BUG=708261 TEST=Test on chromecast and Android. ========== to ========== Add EME contentType checks for mp2t We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. The patch adds the supporting for mp2t avc. BUG=708261 TEST=Test on chromecast and Android. ==========
yucliu@chromium.org changed reviewers: + chcunningham@chromium.org, servolk@chromium.org, slan@chromium.org, xhwang@chromium.org
LGTM, but I would wait for xhwang to also take a look
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org
+ddorwin for discussion. WDYT? The pattern EME_CODEC_COMMON_* was first added for VP9 in MP4 support: https://cs.chromium.org/chromium/src/media/base/key_systems.cc?rcl=7afec42068... It means that the codec can be supported in multiple containers (e.g. WebM and MP4). In the EME_CODEC_COMMON_VP9 case, it also uses a new codec string vp09. That's why we have both EME_CODEC_COMMON_VP9 and EME_CODEC_WEBM_VP9. Now in the CL we are adding another mime type "video/mp2t", and now "COMMON" doesn't apply to all mime types any more, and it means more like "shared" among multiple mime types, which is a bit confusing. Given the fact that we are transitioning to a world that one codec may be used in multiple mime types, I wonder whether limiting codecs to a container type makes sense any more. For example, EME_CODEC_WEBM_VP9 could just be EME_CODEC_VP9, and that can be used in multiple mime types, and when we define masks like EME_CODEC_WEBM_VIDEO_ALL, we can pick what codecs are supported for that specific mime/container type.
Another option is to list all mime/container types supported by the codec, e.g. instead of EME_CODEC_COMMON_VP9, we have EME_CODEC_WEBM_MP4_VP9, but this will be harder to maintain and keeping the codec and container separate seems more reasonable to me.
On 2017/05/15 18:36:40, xhwang wrote: > +ddorwin for discussion. WDYT? > > The pattern EME_CODEC_COMMON_* was first added for VP9 in MP4 support: > https://cs.chromium.org/chromium/src/media/base/key_systems.cc?rcl=7afec42068... > > It means that the codec can be supported in multiple containers (e.g. WebM and > MP4). In the EME_CODEC_COMMON_VP9 case, it also uses a new codec string vp09. > That's why we have both EME_CODEC_COMMON_VP9 and EME_CODEC_WEBM_VP9. > > Now in the CL we are adding another mime type "video/mp2t", and now "COMMON" > doesn't apply to all mime types any more, and it means more like "shared" among > multiple mime types, which is a bit confusing. > > Given the fact that we are transitioning to a world that one codec may be used > in multiple mime types, I wonder whether limiting codecs to a container type > makes sense any more. For example, EME_CODEC_WEBM_VP9 could just be > EME_CODEC_VP9, and that can be used in multiple mime types, and when we define > masks like EME_CODEC_WEBM_VIDEO_ALL, we can pick what codecs are supported for > that specific mime/container type. Should we split container type and codec type? Do we have cases where the decoder supports codec in one container but not in the other container?
On 2017/05/15 18:47:01, yucliu1 wrote: > On 2017/05/15 18:36:40, xhwang wrote: > > +ddorwin for discussion. WDYT? > > > > The pattern EME_CODEC_COMMON_* was first added for VP9 in MP4 support: > > > https://cs.chromium.org/chromium/src/media/base/key_systems.cc?rcl=7afec42068... > > > > It means that the codec can be supported in multiple containers (e.g. WebM and > > MP4). In the EME_CODEC_COMMON_VP9 case, it also uses a new codec string vp09. > > That's why we have both EME_CODEC_COMMON_VP9 and EME_CODEC_WEBM_VP9. > > > > Now in the CL we are adding another mime type "video/mp2t", and now "COMMON" > > doesn't apply to all mime types any more, and it means more like "shared" > among > > multiple mime types, which is a bit confusing. > > > > Given the fact that we are transitioning to a world that one codec may be used > > in multiple mime types, I wonder whether limiting codecs to a container type > > makes sense any more. For example, EME_CODEC_WEBM_VP9 could just be > > EME_CODEC_VP9, and that can be used in multiple mime types, and when we define > > masks like EME_CODEC_WEBM_VIDEO_ALL, we can pick what codecs are supported for > > that specific mime/container type. > > Should we split container type and codec type? That's what I am proposing here.. but that doesn't mean you can put a codec in any container. > Do we have cases where the > decoder supports codec in one container but not in the other container? Internally our decoders don't care about container types, but I don't think we can put a codec in any container, e.g. avc1 in webm.
On 2017/05/15 18:56:50, xhwang wrote: > On 2017/05/15 18:47:01, yucliu1 wrote: > > On 2017/05/15 18:36:40, xhwang wrote: > > > +ddorwin for discussion. WDYT? > > > > > > The pattern EME_CODEC_COMMON_* was first added for VP9 in MP4 support: > > > > > > https://cs.chromium.org/chromium/src/media/base/key_systems.cc?rcl=7afec42068... > > > > > > It means that the codec can be supported in multiple containers (e.g. WebM > and > > > MP4). In the EME_CODEC_COMMON_VP9 case, it also uses a new codec string > vp09. > > > That's why we have both EME_CODEC_COMMON_VP9 and EME_CODEC_WEBM_VP9. > > > > > > Now in the CL we are adding another mime type "video/mp2t", and now "COMMON" > > > doesn't apply to all mime types any more, and it means more like "shared" > > among > > > multiple mime types, which is a bit confusing. > > > > > > Given the fact that we are transitioning to a world that one codec may be > used > > > in multiple mime types, I wonder whether limiting codecs to a container type > > > makes sense any more. For example, EME_CODEC_WEBM_VP9 could just be > > > EME_CODEC_VP9, and that can be used in multiple mime types, and when we > define > > > masks like EME_CODEC_WEBM_VIDEO_ALL, we can pick what codecs are supported > for > > > that specific mime/container type. > > > > Should we split container type and codec type? > > That's what I am proposing here.. but that doesn't mean you can put a codec in > any container. > > > Do we have cases where the > > decoder supports codec in one container but not in the other container? > > Internally our decoders don't care about container types, but I don't think we > can put a codec in any container, e.g. avc1 in webm. I agree. The key system doesn't need to care about the container also, since the parser already knows whether we can put the codec in the container. In other words, we can check the container type before we query key system.
> > > Now in the CL we are adding another mime type "video/mp2t", and now "COMMON" > > > doesn't apply to all mime types any more, and it means more like "shared" > > > among mime types, which is a bit confusing. To me "common" means the same thing in my CL and in this CL. COMMON in the VP9 CL meant "the vp9 string that is common to multiple containers"... webm and mp4. In this CL, COMMON has the same meaning, just for different mix of codec and containers. The only difference is I was unable to simply replace the WEB_VP9 string in my CL because its needed to maintain backward compat of the old simpler vp9 string. But that is a detail that doesn't change the meaning of "commmon" imho. > > > Given the fact that we are transitioning to a world that one codec may be > used > > > in multiple mime types, I wonder whether limiting codecs to a container type > > > makes sense any more. For example, EME_CODEC_WEBM_VP9 could just be > > > EME_CODEC_VP9, and that can be used in multiple mime types, and when we > define > > > masks like EME_CODEC_WEBM_VIDEO_ALL, we can pick what codecs are supported > for > > > that specific mime/container type. I think this limitation is still real. We can only demux certain combos and we want to enforce that somewhere. I'm open to changing where/how we enforce it, but the need is still there.
> I agree. The key system doesn't need to care about the container also, since the > parser already knows whether we can put the codec in the container. In other > words, we can check the container type before we query key system. This makes sense to me too - AFAIK the CDM contract is such that we always do demuxing prior to handing to CDM right?
On 2017/05/15 19:14:29, chcunningham wrote: > > I agree. The key system doesn't need to care about the container also, since > the > > parser already knows whether we can put the codec in the container. In other > > words, we can check the container type before we query key system. > > This makes sense to me too - AFAIK the CDM contract is such that we always do > demuxing prior to handing to CDM right? CDM is used by decoder, which is after demuxing. If splitting codec and container type sounds good, I'll remove container from AVC1 in this patch and change others in a later patch.
On 2017/05/16 22:38:39, yucliu1 wrote: > On 2017/05/15 19:14:29, chcunningham wrote: > > > I agree. The key system doesn't need to care about the container also, since > > the > > > parser already knows whether we can put the codec in the container. In other > > > words, we can check the container type before we query key system. > > > > This makes sense to me too - AFAIK the CDM contract is such that we always do > > demuxing prior to handing to CDM right? > > CDM is used by decoder, which is after demuxing. > > If splitting codec and container type sounds good, I'll remove container from > AVC1 in this patch and change others in a later patch. I'll take another closer look to confirm what I proposed. And reply here tomorrow.
I think this enum was added for the prefixed EME implementation and before we had robust string checks against MimeUtil. At that time, a) this was the only way of checking container-codec compatibility and b) a codec was always associated with a single container so CONTAINER appeared in all the constants used to build the container's masks. I think we now use MimeUtil to handle (a) (I'm assuming so below), and I don't think we need to maintain the naming in (b) now. Other thoughts: * I don't think COMMON makes sense here. * COMMON_VP9 should be just VP9, and WEBM_VP9 should be VP9_LEGACY. * But really, CDMs only need to support the VP9 codec and don't care about the string format. * Why does cdm_message_filter_android.cc pass containers when querying codecs? Also, it doesn't query VP9 in MP4. * I believe AndroidPlatformKeySystemProperties::IsSupportedInitDataType is abusing these enums, and this logic may no longer be correct with these "common" types. See https://crbug.com/589521. Related to this, there are some things we should clean up in key_systems.cc: * kCodecStrings probably doesn't need separate enum values. It is used to convert strings to an enum. * We can remove vp8.0 and vp9.0 from that table because the strings are stripped. * That table should be renamed to include "Stripped" or similar. * The name of codec_string_map_ and parameters that are passed to methods that use it should also be updated. Rather than just changing AVC1 in this patch, you should probably clean up at least some these things before making logic changes. https://codereview.chromium.org/2881443002/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/2881443002/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:70: {"avc1", EME_CODEC_COMMON_AVC1}, // AVC1 for MP4 and MP2T The additional comment text seems unnecessary.
P.S. It would be better if KeySystems didn't have to deal with codec strings and could, for example, rely on MimeUtil. Unfortunately, its Codecs enum is not public. See also https://crbug.com/417440.
On 2017/05/17 16:50:17, ddorwin wrote: > I think this enum was added for the prefixed EME implementation and before we > had robust string checks against MimeUtil. At that time, a) this was the only > way of checking container-codec compatibility and b) a codec was always > associated with a single container so CONTAINER appeared in all the constants > used to build the container's masks. I think we now use MimeUtil to handle (a) > (I'm assuming so below), and I don't think we need to maintain the naming in (b) > now. > > Other thoughts: > * I don't think COMMON makes sense here. > * COMMON_VP9 should be just VP9, and WEBM_VP9 should be VP9_LEGACY. > * But really, CDMs only need to support the VP9 codec and don't care about the > string format. > * Why does cdm_message_filter_android.cc pass containers when querying codecs? > Also, it doesn't query VP9 in MP4. I think Android is treating the container type as init data type. It's checking the supported init data type here. > * I believe AndroidPlatformKeySystemProperties::IsSupportedInitDataType is > abusing these enums, and this logic may no longer be correct with these "common" > types. See https://crbug.com/589521. Sounds like we should ask key system to check supported init data type, instead of container type. > > Related to this, there are some things we should clean up in key_systems.cc: > * kCodecStrings probably doesn't need separate enum values. It is used to > convert strings to an enum. > * We can remove vp8.0 and vp9.0 from that table because the strings are > stripped. > * That table should be renamed to include "Stripped" or similar. > * The name of codec_string_map_ and parameters that are passed to methods that > use it should also be updated. > > > Rather than just changing AVC1 in this patch, you should probably clean up at > least some these things before making logic changes. > > https://codereview.chromium.org/2881443002/diff/1/media/base/key_systems.cc > File media/base/key_systems.cc (right): > > https://codereview.chromium.org/2881443002/diff/1/media/base/key_systems.cc#n... > media/base/key_systems.cc:70: {"avc1", EME_CODEC_COMMON_AVC1}, // AVC1 for MP4 > and MP2T > The additional comment text seems unnecessary.
Description was changed from ========== Add EME contentType checks for mp2t We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. The patch adds the supporting for mp2t avc. BUG=708261 TEST=Test on chromecast and Android. ========== to ========== Add missing EME contentType checks for mp2t+avc1 (all platforms) and mp4+vp9 (Android): 1. Mp2t: We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. 2. EmeCodec renaming, since CDM doesn't need to care about containers: EME_CODEC_COMMON_VP9 -> EME_CODEC_VP9, EME_CODEC_WEBM_VP9 -> EME_CODEC_VP9_LEGACY, EME_CODEC_MP4_AVC1 -> EME_CODEC_AVC1 (Other names will be changed later) 3. Rename kCodecStrings to kStrippedCodecStrings for better naming. BUG=708261 TEST=Test on chromecast and Android. ==========
Description was changed from ========== Add missing EME contentType checks for mp2t+avc1 (all platforms) and mp4+vp9 (Android): 1. Mp2t: We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. 2. EmeCodec renaming, since CDM doesn't need to care about containers: EME_CODEC_COMMON_VP9 -> EME_CODEC_VP9, EME_CODEC_WEBM_VP9 -> EME_CODEC_VP9_LEGACY, EME_CODEC_MP4_AVC1 -> EME_CODEC_AVC1 (Other names will be changed later) 3. Rename kCodecStrings to kStrippedCodecStrings for better naming. BUG=708261 TEST=Test on chromecast and Android. ========== to ========== Add missing EME contentType checks Add missing EME contentType checks for mp2t+avc1 (all platforms) and mp4+vp9 (Android): 1. Mp2t: We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. 2. EmeCodec renaming, since CDM doesn't need to care about containers: EME_CODEC_COMMON_VP9 -> EME_CODEC_VP9, EME_CODEC_WEBM_VP9 -> EME_CODEC_VP9_LEGACY, EME_CODEC_MP4_AVC1 -> EME_CODEC_AVC1 (Other names will be changed later) 3. Rename kCodecStrings to kStrippedCodecStrings for better naming. BUG=708261 TEST=Test on chromecast and Android. ==========
Description was changed from ========== Add missing EME contentType checks Add missing EME contentType checks for mp2t+avc1 (all platforms) and mp4+vp9 (Android): 1. Mp2t: We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. 2. EmeCodec renaming, since CDM doesn't need to care about containers: EME_CODEC_COMMON_VP9 -> EME_CODEC_VP9, EME_CODEC_WEBM_VP9 -> EME_CODEC_VP9_LEGACY, EME_CODEC_MP4_AVC1 -> EME_CODEC_AVC1 (Other names will be changed later) 3. Rename kCodecStrings to kStrippedCodecStrings for better naming. BUG=708261 TEST=Test on chromecast and Android. ========== to ========== Add missing EME contentType checks Add missing EME contentType checks for mp2t+avc1 (all platforms) and mp4+vp9 (Android): 1. Mp2t: We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. 2. EmeCodec renaming, since CDM doesn't need to care about containers: EME_CODEC_COMMON_VP9 -> EME_CODEC_VP9, EME_CODEC_WEBM_VP9 -> EME_CODEC_VP9_LEGACY, EME_CODEC_MP4_AVC1 -> EME_CODEC_AVC1 (Other names will be changed later) 3. Rename kCodecStrings to kStrippedCodecStrings for better naming. BUG=708261 TEST=Test on chromecast and Android. ==========
1. Rename COMMON_VP9 to VP9, WEBM_VP9 to VP9_LEGACY, COMMON_AVC1 to AVC1 2. Add missing VP9 check for mp4 in Android 3. Rename kCodecStrings to kStrippedCodecStrings
yucliu: Thanks for the clean up! I just filed https://bugs.chromium.org/p/chromium/issues/detail?id=724362 to track the clean up work summarized by ddorwin. At the same time, let's not mix unrelated changes into one CL, which will make it harder to review/track. To unblock your work, I think you can just land PS1, without renaming EME_CODEC_MP4_AVC1 to EME_CODEC_COMMON_AVC1. You can add comment/TODO at places where this might be confusing. Then, we can make a series of clean-ups to improve things: 1. fix "stripped" related code/names. 2. Drop container type in EmeCodecs, e.g. EME_CODEC_MP4_AVC1 -> EME_CODEC_AVC1 3, Fix codec and init data type query on Android. This will also fix the problem where we don't support VP9+MP4 on Android. WDYT?
Description was changed from ========== Add missing EME contentType checks Add missing EME contentType checks for mp2t+avc1 (all platforms) and mp4+vp9 (Android): 1. Mp2t: We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. 2. EmeCodec renaming, since CDM doesn't need to care about containers: EME_CODEC_COMMON_VP9 -> EME_CODEC_VP9, EME_CODEC_WEBM_VP9 -> EME_CODEC_VP9_LEGACY, EME_CODEC_MP4_AVC1 -> EME_CODEC_AVC1 (Other names will be changed later) 3. Rename kCodecStrings to kStrippedCodecStrings for better naming. BUG=708261 TEST=Test on chromecast and Android. ========== to ========== Add EME contentType checks for mp2t We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. The patch adds the supporting for mp2t avc. BUG=708261 TEST=Test on chromecast and Android. ==========
On 2017/05/19 05:08:58, xhwang wrote: > yucliu: Thanks for the clean up! > > I just filed https://bugs.chromium.org/p/chromium/issues/detail?id=724362 to > track the clean up work summarized by ddorwin. At the same time, let's not mix > unrelated changes into one CL, which will make it harder to review/track. > > To unblock your work, I think you can just land PS1, without renaming > EME_CODEC_MP4_AVC1 to EME_CODEC_COMMON_AVC1. You can add comment/TODO at places > where this might be confusing. > > Then, we can make a series of clean-ups to improve things: > 1. fix "stripped" related code/names. > 2. Drop container type in EmeCodecs, e.g. EME_CODEC_MP4_AVC1 -> EME_CODEC_AVC1 > 3, Fix codec and init data type query on Android. This will also fix the problem > where we don't support VP9+MP4 on Android. > > WDYT? Restore old patch and name. Add some comments.
Thanks! LGTM
The CQ bit was checked by yucliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org Link to the patchset: https://codereview.chromium.org/2881443002/#ps80001 (title: "Name and comment")
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by yucliu@chromium.org
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yucliu@chromium.org
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": 80001, "attempt_start_ts": 1495471640271090, "parent_rev": "2ac0511d28994ccd83280e6c93f773ed6876a237", "commit_rev": "a26af12f05559a607d50c22d94135b18902167eb"}
Message was sent while issue was closed.
Description was changed from ========== Add EME contentType checks for mp2t We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. The patch adds the supporting for mp2t avc. BUG=708261 TEST=Test on chromecast and Android. ========== to ========== Add EME contentType checks for mp2t We don't have 'mp2t/audio' for audio streams. For video streams, stream parser supports encrypted AVC stream. The patch adds the supporting for mp2t avc. BUG=708261 TEST=Test on chromecast and Android. Review-Url: https://codereview.chromium.org/2881443002 Cr-Commit-Position: refs/heads/master@{#473633} Committed: https://chromium.googlesource.com/chromium/src/+/a26af12f05559a607d50c22d9413... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/a26af12f05559a607d50c22d9413... |