Bug 233441

Summary: ScalableImageDecoder should be using SharedBuffer instead of DataSegment
Product: WebKit Reporter: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Component: ImagesAssignee: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, sabouhallawa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=230662
Bug Depends on: 233030    
Bug Blocks: 232424    
Attachments:
Description Flags
Patch
none
Patch
none
Test
none
Patch none

Jean-Yves Avenard [:jya]
Reported 2021-11-23 00:20:43 PST
In https://trac.webkit.org/changeset/251836/webkit DataSegment was used over SharedBuffer because SharedBuffer didn't use thread-safe refcounting. However, since bug 230662, this is no longer the case. Once bug 233030 lands, SharedBuffer becomes immutable once created and can be shared across threads. ScalableImageDecoder currently takes a SharedBuffer, flatten it through the call to SharedBuffer::data() and then take a reference to the inner DataSegment. ScalableImageDecoder should be re-written to use SharedBuffer so that we can do without the copy/memory allocation required by the flattening of the SharedBuffer.
Attachments
Patch (13.18 KB, patch)
2021-12-23 05:35 PST, Jean-Yves Avenard [:jya] 		no flags
DetailsFormatted DiffDiff
Patch (13.09 KB, patch)
2021-12-23 14:24 PST, Jean-Yves Avenard [:jya] 		no flags
DetailsFormatted DiffDiff
Test (2.97 KB, patch)
2021-12-23 15:19 PST, Jean-Yves Avenard [:jya] 		no flags
DetailsFormatted DiffDiff
Patch (12.24 KB, patch)
2021-12-23 17:38 PST, Jean-Yves Avenard [:jya] 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2021-11-30 00:21:25 PST
<rdar://problem/85854204>
Jean-Yves Avenard [:jya]
Comment 2 2021-12-23 05:35:44 PST
Created attachment 447874 [details] Patch
Jean-Yves Avenard [:jya]
Comment 3 2021-12-23 14:24:53 PST
Created attachment 447909 [details] Patch rebase
Jean-Yves Avenard [:jya]
Comment 4 2021-12-23 15:19:57 PST
Created attachment 447912 [details] Test Checking that those errors aren't happening with unpatched tree.
Jean-Yves Avenard [:jya]
Comment 5 2021-12-23 17:38:14 PST
Created attachment 447924 [details] Patch Use DataSegment for ImageBackingStore
Darin Adler
Comment 6 2022-01-01 01:26:44 PST
Comment on attachment 447924 [details] Patch If SharedBuffer is intrinsically immutable, then I think we should be using RefPtr<SharedBuffer> rather than RefPtr<const SharedBuffer>. Saying const all the time doesn’t have any meaning, right?
Jean-Yves Avenard [:jya]
Comment 7 2022-01-01 12:46:36 PST
(In reply to Darin Adler from comment #6) > Comment on attachment 447924 [details] > Patch > > If SharedBuffer is intrinsically immutable, then I think we should be using > RefPtr<SharedBuffer> rather than RefPtr<const SharedBuffer>. Saying const > all the time doesn’t have any meaning, right? SharedBuffer still does have non const method such as takeData and in bug 232424 all SharedBuffer are passed as const SharedBuffer& By making it a RefPtr<const SharedBuffer> I can remove all the required const cast when taking a strong reference to that buffer
EWS
Comment 8 2022-01-01 16:20:07 PST
Committed r287507 (245642@main): <https://commits.webkit.org/245642@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447924 [details].
Note You need to log in before you can comment on or make changes to this bug.