Bug 151221 – Adopt AVContentKeySession

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Adopt AVContentKeySession

[Jer Noble]

Created attachment 265432 [details]
WIP

[Jer Noble]

Created attachment 266468 [details]
Patch

[Jon Lee]

rdar://problem/20001946

[Jer Noble]

Created attachment 266559 [details]
Patch

[Eric Carlson]

Comment on attachment 266559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266559&action=review

> Source/WebCore/ChangeLog:53
> +        (-[CDMSessionAVStreamSessionObserver initWithParent:]): Moved from AVSessionMediaSourceAVFObjC.

Don't you mean CDMSessionMediaSourceAVFObjCObserver?

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:121
> +        if (errorCode) {

Oops :-O

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.h:54
> +    virtual RefPtr<Uint8Array> generateKeyRequest(const String& mimeType, Uint8Array* initData, String& destinationURL, unsigned short& errorCode, unsigned long& systemCode) override;
> +    virtual void releaseKeys() override;
> +    virtual bool update(Uint8Array*, RefPtr<Uint8Array>& nextMessage, unsigned short& errorCode, unsigned long& systemCode) override;

Nit: why name initData and not the first parameter to update? Why name nextMessage?

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.h:82
> +inline CDMSessionAVContentKeySession* toCDMSessionAVContentKeySession(CDMSession* session)
> +{
> +    if (!session || session->type() != CDMSessionTypeAVContentKeySession)
> +        return nullptr;
> +    return static_cast<CDMSessionAVContentKeySession*>(session);

Is it ever OK for someone to call this with anything except a CDMSessionTypeAVContentKeySession? If not, you should add an ASSERT.

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:97
> +
> +

Nit: don't need two blank lines.

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:171
> +    if (equalIgnoringCase(mimeType, "keyrelease")) {
> +        m_mode = KeyRelease;
> +        return generateKeyReleaseMessage(errorCode, systemCode);
> +    }
> +
> +    if (!m_certificate) {
> +        String certificateString(ASCIILiteral("certificate"));
> +        RefPtr<Uint8Array> array = Uint8Array::create(certificateString.length());
> +        for (unsigned i = 0, length = certificateString.length(); i < length; ++i)
> +            array->set(i, certificateString[i]);
> +        return array;
> +    }

It looks like this is exactly the same in CDMSessionAVStreamSession, can you put it in the base class?

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:183
> +    if (hasContentKeySession()) {

Nit: this could be an early return.

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:229
> +static bool isEqual(Uint8Array* data, const char* literal)
> +{
> +    ASSERT(data);
> +    ASSERT(literal);
> +    unsigned length = data->length();
> +
> +    for (unsigned i = 0; i < length; ++i) {
> +        if (!literal[i])
> +            return false;
> +
> +        if (data->item(i) != static_cast<uint8_t>(literal[i]))
> +            return false;
> +    }
> +    return !literal[length];
> +}

It looks like this is exactly the same in CDMSessionAVStreamSession, can you put it in the base class?

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:286
> +        NSData* requestData = [m_keyRequest contentKeyRequestDataForApp:certificateData.get() contentIdentifier:initData.get() options:options.get() error:nil];

Doesn't this leak?

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:319
> +        systemCode = '!mor';

Can you return '!mor' from a method in the base class?

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.h:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

:-O

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.h:52
> +    virtual RefPtr<Uint8Array> generateKeyRequest(const String& mimeType, Uint8Array* initData, String& destinationURL, unsigned short& errorCode, unsigned long& systemCode) override;
> +    virtual void releaseKeys() override;
> +    virtual bool update(Uint8Array*, RefPtr<Uint8Array>& nextMessage, unsigned short& errorCode, unsigned long& systemCode) override;

Ditto my nit from the other class header.

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

:-O

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm:140
> +void CDMSessionAVStreamSession::releaseKeys()

This whole method is annoyingly similar to CDMSessionAVContentKeySession::releaseKeys...

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm:216
> +    bool shouldGenerateKeyRequest = !m_certificate || isEqual(key, "renew");
> +    if (!m_certificate) {
> +        LOG(Media, "CDMSessionAVStreamSession::update(%p) - certificate data", this);
> +
> +        m_certificate = key;
> +    }
> +
> +    if (isEqual(key, "acknowledged")) {
> +        LOG(Media, "CDMSessionAVStreamSession::update(%p) - acknowleding secure stop message", this);
> +
> +        if (!m_expiredSession) {
> +            errorCode = MediaPlayer::InvalidPlayerState;
> +            return false;
> +        }
> +
> +        RetainPtr<NSData> certificateData = adoptNS([[NSData alloc] initWithBytes:m_certificate->data() length:m_certificate->length()]);
> +
> +        if ([getAVStreamSessionClass() respondsToSelector:@selector(removePendingExpiredSessionReports:withAppIdentifier:storageDirectoryAtURL:)])
> +            [getAVStreamSessionClass() removePendingExpiredSessionReports:@[m_expiredSession.get()] withAppIdentifier:certificateData.get() storageDirectoryAtURL:[NSURL fileURLWithPath:storagePath()]];
> +        m_expiredSession = nullptr;
> +        return true;
> +    }
> +
> +    if (m_mode == KeyRelease)
> +        return false;

It looks like this is exactly the same as the first part of CDMSessionAVContentKeySession::update. Can you put it into a shared method in the base class?

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm:237
> +
> +

Nit: two blank lines.

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionMediaSourceAVFObjC.h:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

2014-2015?

> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionMediaSourceAVFObjC.mm:2
> - * Copyright (C) 2014 Apple Inc. All rights reserved.
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

Ditto.

[Jer Noble]

Comment on attachment 266559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266559&action=review

>> Source/WebCore/ChangeLog:53
>> +        (-[CDMSessionAVStreamSessionObserver initWithParent:]): Moved from AVSessionMediaSourceAVFObjC.
> 
> Don't you mean CDMSessionMediaSourceAVFObjCObserver?

I actually meant the source file, but you're right.

>> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.h:54
>> +    virtual bool update(Uint8Array*, RefPtr<Uint8Array>& nextMessage, unsigned short& errorCode, unsigned long& systemCode) override;
> 
> Nit: why name initData and not the first parameter to update? Why name nextMessage?

Copy pasta.

>> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.h:82
>> +    return static_cast<CDMSessionAVContentKeySession*>(session);
> 
> Is it ever OK for someone to call this with anything except a CDMSessionTypeAVContentKeySession? If not, you should add an ASSERT.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:171
>> +    }
> 
> It looks like this is exactly the same in CDMSessionAVStreamSession, can you put it in the base class?

That may be hard; I'll have to do some refactoring, but i'll do that in a follow up.

>> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:229
>> +}
> 
> It looks like this is exactly the same in CDMSessionAVStreamSession, can you put it in the base class?

Not without making it a class method.  I'll see what I can do.

>> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:286
>> +        NSData* requestData = [m_keyRequest contentKeyRequestDataForApp:certificateData.get() contentIdentifier:initData.get() options:options.get() error:nil];
> 
> Doesn't this leak?

The return value is autoreleased, so no.

>> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:319
>> +        systemCode = '!mor';
> 
> Can you return '!mor' from a method in the base class?

I can probably refactor things, or add a constant static in the base class.

>> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.h:2
>> + * Copyright (C) 2014 Apple Inc. All rights reserved.
> 
> :-O

Whoops!

>> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm:216
>> +        return false;
> 
> It looks like this is exactly the same as the first part of CDMSessionAVContentKeySession::update. Can you put it into a shared method in the base class?

Same as above; i'll have do so some refactoring to get there.

[Jer Noble]

Committed r193479: <http://trac.webkit.org/changeset/193479>