Bug 214928 - Support HDR decode in SW VP9
Summary: Support HDR decode in SW VP9
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-29 11:57 PDT by Jer Noble
Modified: 2020-07-29 18:01 PDT (History)
8 users (show)

See Also:


Attachments
Patch (24.17 KB, patch)
2020-07-29 12:07 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff
Patch (36.91 KB, patch)
2020-07-29 12:19 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing. (37.21 KB, patch)
2020-07-29 12:51 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (37.25 KB, patch)
2020-07-29 14:17 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (37.22 KB, patch)
2020-07-29 14:37 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].