Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2494983002:
media: Supports Clear Key key system for mojo CDM/Renderer combo (Closed)
|
Created:
4 years, 1 month ago by xhwang Modified:
4 years, 1 month ago CC:
chromium-reviews, qsr+mojo_chromium.org, feature-media-reviews_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, alokp+watch_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Supports Clear Key key system for mojo CDM/Renderer combo
When both mojo CDM and mojo Renderer are used, do not create
AesDecryptor in the render process since it cannot be used by the remote
renderer. Instead, always create the mojo CDM, and the MojoMediaClient
can choose how to implement it.
BUG=441957, 641559
TEST=This CL enables more tests.
Committed: https://crrev.com/4155c401799a814cefc13f5b2589034dde217651
Cr-Commit-Position: refs/heads/master@{#431705}
Patch Set 1 #Patch Set 2 : fix comments #
Total comments: 4
Messages
Total messages: 32 (18 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: 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...)
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
PTAL
Description was changed from ========== media: Supports Clear Key key system for mojo CDM/Renderer combo When both mojo CDM and mojo Renderer are used, do not create AesDecryptor in the render process since it cannot be used by the remote renderer. Instead, always create the mojo CDM, and the MojoMediaClient can choose how to implement it. BUG=641559 TEST=This CL enables more tests. ========== to ========== media: Supports Clear Key key system for mojo CDM/Renderer combo When both mojo CDM and mojo Renderer are used, do not create AesDecryptor in the render process since it cannot be used by the remote renderer. Instead, always create the mojo CDM, and the MojoMediaClient can choose how to implement it. BUG=441957,641559 TEST=This CL enables more tests. ==========
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...
See some related thoughts at https://bugs.chromium.org/p/chromium/issues/detail?id=441957#c23
xhwang@chromium.org changed reviewers: + yucliu@chromium.org
yucliu: FYI
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2494983002/diff/20001/media/mojo/clients/mojo... File media/mojo/clients/mojo_cdm_factory.cc (right): https://codereview.chromium.org/2494983002/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm_factory.cc:49: #if !defined(ENABLE_MOJO_RENDERER) Can we have mojo cdm without mojo renderer?
https://codereview.chromium.org/2494983002/diff/20001/media/mojo/clients/mojo... File media/mojo/clients/mojo_cdm_factory.cc (right): https://codereview.chromium.org/2494983002/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm_factory.cc:49: #if !defined(ENABLE_MOJO_RENDERER) On 2016/11/11 21:44:38, yucliu1 wrote: > Can we have mojo cdm without mojo renderer? Yes, MojoCdm can work with the existing local RendererImpl through MojoDecryptor. This is the plan for desktop CDM.
This change is fine since it doesn't actually add any functionality, but it assumes that Clear Key is being implemented remotely. It would be nice if we could ensure that AesDecryptor was in sandboxed process, but that's not practical. It will be up to each implementation to ensure they implementation mitigates any security concerns, such as via sandboxing. LGTM
On 2016/11/11 23:18:43, ddorwin wrote: > This change is fine since it doesn't actually add any functionality, but it > assumes that Clear Key is being implemented remotely. It would be nice if we > could ensure that AesDecryptor was in sandboxed process, but that's not > practical. It will be up to each implementation to ensure they implementation > mitigates any security concerns, such as via sandboxing. > > LGTM One more question, is it possible to add a getter for the SymmetricKey in AesDecryptor?
On 2016/11/12 00:19:09, yucliu1 wrote: > On 2016/11/11 23:18:43, ddorwin wrote: > > This change is fine since it doesn't actually add any functionality, but it > > assumes that Clear Key is being implemented remotely. It would be nice if we > > could ensure that AesDecryptor was in sandboxed process, but that's not > > practical. It will be up to each implementation to ensure they implementation > > mitigates any security concerns, such as via sandboxing. > > > > LGTM > > One more question, is it possible to add a getter for the SymmetricKey in > AesDecryptor? There's no use case for such a getter today in Chromium.. If you feel there's a generic use case for it, please feel free to file a bug and we can start from there. Also, if you feel the current performance of AesDecryptor is not good, please also file a bug and we can improve it. Thanks!
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/12 00:24:15, xhwang wrote: > On 2016/11/12 00:19:09, yucliu1 wrote: > > On 2016/11/11 23:18:43, ddorwin wrote: > > > This change is fine since it doesn't actually add any functionality, but it > > > assumes that Clear Key is being implemented remotely. It would be nice if we > > > could ensure that AesDecryptor was in sandboxed process, but that's not > > > practical. It will be up to each implementation to ensure they > implementation > > > mitigates any security concerns, such as via sandboxing. > > > > > > LGTM > > > > One more question, is it possible to add a getter for the SymmetricKey in > > AesDecryptor? > > There's no use case for such a getter today in Chromium.. If you feel there's a > generic use case for it, please feel free to file a bug and we can start from > there. Also, if you feel the current performance of AesDecryptor is not good, > please also file a bug and we can improve it. Thanks! Thanks. LGTM and one more problem I met during development (this may be fixed in some other patch).
https://codereview.chromium.org/2494983002/diff/20001/media/mojo/clients/mojo... File media/mojo/clients/mojo_cdm_factory.cc (right): https://codereview.chromium.org/2494983002/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm_factory.cc:52: new AesDecryptor(security_origin, session_message_cb, session_closed_cb, I remember the AesDecryptor::GetCdmId returns kInvalidCdmId which may breaks some assumption: https://cs.chromium.org/chromium/src/media/mojo/clients/mojo_audio_decoder.cc... https://cs.chromium.org/chromium/src/media/filters/gpu_video_decoder.cc?sq=pa...
https://codereview.chromium.org/2494983002/diff/20001/media/mojo/clients/mojo... File media/mojo/clients/mojo_cdm_factory.cc (right): https://codereview.chromium.org/2494983002/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm_factory.cc:52: new AesDecryptor(security_origin, session_message_cb, session_closed_cb, On 2016/11/12 00:26:24, yucliu1 wrote: > I remember the AesDecryptor::GetCdmId returns kInvalidCdmId which may breaks > some assumption: > > https://cs.chromium.org/chromium/src/media/mojo/clients/mojo_audio_decoder.cc... > > https://cs.chromium.org/chromium/src/media/filters/gpu_video_decoder.cc?sq=pa... These are all remote decoders, which will not work with AesDecryptor directly. In this cease, usually we use DecryptingDemuxerStream to decrypt in the render process directly, then what the decoders get will be clear (unencrypted) stream: https://cs.chromium.org/chromium/src/media/filters/decoder_selector.cc?rcl=0&...
Message was sent while issue was closed.
Description was changed from ========== media: Supports Clear Key key system for mojo CDM/Renderer combo When both mojo CDM and mojo Renderer are used, do not create AesDecryptor in the render process since it cannot be used by the remote renderer. Instead, always create the mojo CDM, and the MojoMediaClient can choose how to implement it. BUG=441957,641559 TEST=This CL enables more tests. ========== to ========== media: Supports Clear Key key system for mojo CDM/Renderer combo When both mojo CDM and mojo Renderer are used, do not create AesDecryptor in the render process since it cannot be used by the remote renderer. Instead, always create the mojo CDM, and the MojoMediaClient can choose how to implement it. BUG=441957,641559 TEST=This CL enables more tests. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== media: Supports Clear Key key system for mojo CDM/Renderer combo When both mojo CDM and mojo Renderer are used, do not create AesDecryptor in the render process since it cannot be used by the remote renderer. Instead, always create the mojo CDM, and the MojoMediaClient can choose how to implement it. BUG=441957,641559 TEST=This CL enables more tests. ========== to ========== media: Supports Clear Key key system for mojo CDM/Renderer combo When both mojo CDM and mojo Renderer are used, do not create AesDecryptor in the render process since it cannot be used by the remote renderer. Instead, always create the mojo CDM, and the MojoMediaClient can choose how to implement it. BUG=441957,641559 TEST=This CL enables more tests. Committed: https://crrev.com/4155c401799a814cefc13f5b2589034dde217651 Cr-Commit-Position: refs/heads/master@{#431705} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4155c401799a814cefc13f5b2589034dde217651 Cr-Commit-Position: refs/heads/master@{#431705} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
