Bug 214928

Summary:

Support HDR decode in SW VP9

Product:

WebKit

Reporter:

Jer Noble <jer.noble>

Component:

New Bugs

Assignee:

Jer Noble <jer.noble>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, eric.carlson, ews-watchlist, glenn, peng.liu6, philipj, sergio, webkit-bug-importer

Priority:

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

Jer Noble

Reported 2020-07-29 11:57:55 PDT

Support HDR decode in SW VP9

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-07-29 11:58:19 PDT

<rdar://problem/66284848>

Jer Noble

Comment 2 2020-07-29 12:07:12 PDT

Created attachment 405480 [details] Patch

Jer Noble

Comment 3 2020-07-29 12:19:46 PDT

Created attachment 405481 [details] Patch

Eric Carlson

Comment 4 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!

Darin Adler

Comment 5 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.

Jer Noble

Comment 6 2020-07-29 12:51:55 PDT

Created attachment 405489 [details] Patch for landing.

Jer Noble

Comment 7 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!

Peng Liu

Comment 8 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.

Jer Noble

Comment 9 2020-07-29 14:17:15 PDT

Created attachment 405499 [details] Patch for landing

Jer Noble

Comment 10 2020-07-29 14:37:14 PDT

Created attachment 405503 [details] Patch for landing

EWS

Comment 11 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].

Note You need to log in before you can comment on or make changes to this bug.