Closed Bug 1188812 Opened 7 years ago Closed 7 years ago

Retrieve CDM CanRender capability and store into MediaInfo for MDSM to create corresponding Sinks

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: kikuo, Assigned: kikuo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Per Bug 1146795 Comment 23, I would like to retrieve CDM CanRender capability from CDMProxy and store into MediaInfo for MDSM.
MDSM could create corresponding media sinks based on this info.
Blocks: 1146795
Hi Chris,

According to Bug 1146795 Comment 24,
I added attribute |mCanExternalRender| in TrackInfo to indicate that if this track is gonna to be (decrypted)/decode/render in external component.
By this attribute, I could create corresponding Media Sink in MDSM (would be a follow-up if Bug 1188268 landed).

Also, I think there's would two follow-up to be created for this bug,

1. The attribute |mCanExternalRender| should also be updated somewhere in non-EME mode for a case(e.g. TV) that clear content is gonna decoded/rendered by other component.

2. If |mCanExternalRender| is true, there should be a way notifying MediaFormatReader not to create platform-decoder, because the decode/render jobs won't be handled by gecko, and EncodedData(non-EME)/EncryptedData(EME) should be queued to EncodedAudioSink/EncrytpedAudioSink (which will be created according to Bug 1188268) and be waiting for consumed.
Assignee: nobody → kikuo
Status: NEW → ASSIGNED
Attachment #8643457 - Flags: review?(cpearce)
Comment on attachment 8643457 [details] [diff] [review]
Obtain_CDM_CanRender_Capability_and_store_in_MediaInfo_255922

Review of attachment 8643457 [details] [diff] [review]:
-----------------------------------------------------------------

r=cpearce with comment & name of new field made consistent.

::: dom/media/MediaInfo.h
@@ +83,5 @@
>    int64_t mDuration;
>    int64_t mMediaTime;
>    CryptoTrack mCrypto;
>  
> +  // True if the track is gonna be (decrypted)/decoded and

The comment here implies the track *will* be rendered externally, but the name implies it *may* be externally rendered. Which is it? The name should match the comment.

If this being true means that the stream is definitely rendered externally, then you should call this mIsRenderedExternally.
Attachment #8643457 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #2)
> Comment on attachment 8643457 [details] [diff] [review]
> Obtain_CDM_CanRender_Capability_and_store_in_MediaInfo_255922
> 
> Review of attachment 8643457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=cpearce with comment & name of new field made consistent.
> 
> ::: dom/media/MediaInfo.h
> @@ +83,5 @@
> >    int64_t mDuration;
> >    int64_t mMediaTime;
> >    CryptoTrack mCrypto;
> >  
> > +  // True if the track is gonna be (decrypted)/decoded and
> 
> The comment here implies the track *will* be rendered externally, but the
> name implies it *may* be externally rendered. Which is it? The name should
> match the comment.
> 
> If this being true means that the stream is definitely rendered externally,
> then you should call this mIsRenderedExternally.

Thanks for the review, the intention of this variable is used for MDSM determining which type of MediaSink to create. The name you suggested "mIsRenderedExternally" is more appropriate. I'll use that. Thanks !
carry r+ from Comment 2, and addressed the issues. (Make variable name and comment consistent)
Attachment #8644160 - Flags: review+
Attachment #8643457 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7bfb63119f1c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.