Bug 214928

Summary: Support HDR decode in SW VP9
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric.carlson, ews-watchlist, glenn, peng.liu6, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
eric.carlson: review+
Patch
none
Patch for landing.
none
Patch for landing
none
Patch for landing none

Description Jer Noble 2020-07-29 11:57:55 PDT 
Support HDR decode in SW VP9
Comment 1 Radar WebKit Bug Importer 2020-07-29 11:58:19 PDT 
<rdar://problem/66284848>
Comment 2 Jer Noble 2020-07-29 12:07:12 PDT 
Created attachment 405480 [details]
Patch
Comment 3 Jer Noble 2020-07-29 12:19:46 PDT 
Created attachment 405481 [details]
Patch
Comment 4 Eric Carlson 2020-07-29 12:45:32 PDT 
Comment on attachment 405480 [details]
Patch

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

r=me once the bots are happy

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:277
> +        CFTypeRef configurationRecord = CFDictionaryGetValue((CFDictionaryRef)extensionAtoms, CFSTR("vpcC"));

`auto configurationRecord = static_cast<CFDataRef>(CFDictionaryGetValue(...`

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:281
> +        auto configurationRecordSize = CFDataGetLength((CFDataRef)configurationRecord);

The cast isn't necessary with the above.

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:285
> +        auto configurationRecordData = CFDataGetBytePtr((CFDataRef)configurationRecord);

Ditto

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:305
> +        CFMutableArrayRef cfPixelFormats = CFArrayCreateMutable(kCFAllocatorDefault, 2, &kCFTypeArrayCallBacks);

'auto cfPixelFormats = ...`

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:310
> +        CFNumberRef borderPixelsValue = createNumber(32);

`auto borderPixelsValue = ...`

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:326
> +        CFDictionaryRef attributes = CFDictionaryCreate(kCFAllocatorDefault, keys, values, std::size(keys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks);

`auto attributes = ...`

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:342
> +    CFRelease(pixelBufferAttributes);

Double release!
Comment 5 Darin Adler 2020-07-29 12:51:54 PDT 
Comment on attachment 405480 [details]
Patch

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

>> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:305
>> +        CFMutableArrayRef cfPixelFormats = CFArrayCreateMutable(kCFAllocatorDefault, 2, &kCFTypeArrayCallBacks);
> 
> 'auto cfPixelFormats = ...`

Why not use CFArrayCreate instead of CRArrayCreateMutable?

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:308
> +        CFRelease(formatNumber);

If this was WebKit code, not libwebtrc, I would suggest using adoptCF instead of CFRelease. It’s hard to get reference counting right the old manual way.
Comment 6 Jer Noble 2020-07-29 12:51:55 PDT 
Created attachment 405489 [details]
Patch for landing.
Comment 7 Jer Noble 2020-07-29 12:53:09 PDT 
Comment on attachment 405480 [details]
Patch

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

>> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:277
>> +        CFTypeRef configurationRecord = CFDictionaryGetValue((CFDictionaryRef)extensionAtoms, CFSTR("vpcC"));
> 
> `auto configurationRecord = static_cast<CFDataRef>(CFDictionaryGetValue(...`

Yeah, much better.

>> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:305
>> +        CFMutableArrayRef cfPixelFormats = CFArrayCreateMutable(kCFAllocatorDefault, 2, &kCFTypeArrayCallBacks);
> 
> 'auto cfPixelFormats = ...`

Ok.

>> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:310
>> +        CFNumberRef borderPixelsValue = createNumber(32);
> 
> `auto borderPixelsValue = ...`

Ok.

>> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:326
>> +        CFDictionaryRef attributes = CFDictionaryCreate(kCFAllocatorDefault, keys, values, std::size(keys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks);
> 
> `auto attributes = ...`

Ok.

>> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP9Decoder.cpp:342
>> +    CFRelease(pixelBufferAttributes);
> 
> Double release!

Ooh, that's bad!
Comment 8 Peng Liu 2020-07-29 13:20:47 PDT 
Comment on attachment 405489 [details]
Patch for landing.

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

> Source/WebCore/ChangeLog:9
> +        CMFormatDescription attached to each incoming video fram.

Nit: s/fram/frame.

> Source/WebCore/ChangeLog:13
> +        in this situations.

Nit: s/this/those.
Comment 9 Jer Noble 2020-07-29 14:17:15 PDT 
Created attachment 405499 [details]
Patch for landing
Comment 10 Jer Noble 2020-07-29 14:37:14 PDT 
Created attachment 405503 [details]
Patch for landing
Comment 11 EWS 2020-07-29 18:01:10 PDT 
Committed r265073: <https://trac.webkit.org/changeset/265073>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405503 [details].