Bug 233441 - ScalableImageDecoder should be using SharedBuffer instead of DataSegment
Summary: ScalableImageDecoder should be using SharedBuffer instead of DataSegment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on: 233030
Blocks: 232424
  Show dependency treegraph
 
Reported: 2021-11-23 00:20 PST by Jean-Yves Avenard [:jya]
Modified: 2022-01-01 16:20 PST (History)
3 users (show)

See Also:


Attachments
Patch (13.18 KB, patch)
2021-12-23 05:35 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (13.09 KB, patch)
2021-12-23 14:24 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Test (2.97 KB, patch)
2021-12-23 15:19 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (12.24 KB, patch)
2021-12-23 17:38 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Yves Avenard [:jya] 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.
Comment 1 Radar WebKit Bug Importer 2021-11-30 00:21:25 PST
<rdar://problem/85854204>
Comment 2 Jean-Yves Avenard [:jya] 2021-12-23 05:35:44 PST
Created attachment 447874 [details]
Patch
Comment 3 Jean-Yves Avenard [:jya] 2021-12-23 14:24:53 PST
Created attachment 447909 [details]
Patch

rebase
Comment 4 Jean-Yves Avenard [:jya] 2021-12-23 15:19:57 PST
Created attachment 447912 [details]
Test

Checking that those errors aren't happening with unpatched tree.
Comment 5 Jean-Yves Avenard [:jya] 2021-12-23 17:38:14 PST
Created attachment 447924 [details]
Patch

Use DataSegment for ImageBackingStore
Comment 6 Darin Adler 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?
Comment 7 Jean-Yves Avenard [:jya] 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
Comment 8 EWS 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].