Bug 233441

Summary:

ScalableImageDecoder should be using SharedBuffer instead of DataSegment

Product:

WebKit

Reporter:

Jean-Yves Avenard [:jya] <jean-yves.avenard>

Component:

Images

Assignee:

Jean-Yves Avenard [:jya] <jean-yves.avenard>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, sabouhallawa, webkit-bug-importer

Priority:

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

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