WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
151221
Adopt AVContentKeySession
https://bugs.webkit.org/show_bug.cgi?id=151221
Summary
Adopt AVContentKeySession
Jer Noble
Reported
2015-11-12 13:58:00 PST
Adopt AVContentKeySession
Attachments
WIP
(106.76 KB, patch)
2015-11-12 14:03 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(105.17 KB, patch)
2015-12-02 13:14 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(107.59 KB, patch)
2015-12-03 14:29 PST
,
Jer Noble
eric.carlson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Jer Noble
Comment 1
2015-11-12 14:03:06 PST
Created
attachment 265432
[details]
WIP
Jer Noble
Comment 2
2015-12-02 13:14:46 PST
Created
attachment 266468
[details]
Patch
Jon Lee
Comment 3
2015-12-03 11:27:09 PST
rdar://problem/20001946
Jer Noble
Comment 4
2015-12-03 14:29:05 PST
Created
attachment 266559
[details]
Patch
Eric Carlson
Comment 5
2015-12-04 14:22:04 PST
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 6
2015-12-04 14:32:45 PST
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
Comment 7
2015-12-04 14:42:02 PST
Committed
r193479
: <
http://trac.webkit.org/changeset/193479
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug