Bug 248185

Summary:	libavif and dav1d should live under ThirdParty/
Product:	WebKit	Reporter:	Michael Catanzaro <mcatanzaro>
Component:	Platform	Assignee:	Nobody <webkit-unassigned>
Status:	RESOLVED FIXED
Severity:	Normal	CC:	ap, mcatanzaro, mmaxfield, ntim, sabouhallawa, webkit-bug-importer
Priority:	P2	Keywords:	InRadar
Version:	WebKit Nightly Build
Hardware:	Unspecified
OS:	All
See Also:	https://bugs.webkit.org/show_bug.cgi?id=246598

Description Michael Catanzaro 2022-11-21 12:04:27 PST

Since 255797@main libavif is now bundled to provide AVIF decoding. Unfortunately, it was imported under Source/WebCore/PAL/libavif rather than Source/ThirdParty. It's important to keep all bundled code under Source/ThirdParty so that we can easily keep track of it, see when the code has been last updated, decide whether to include or exclude it from release tarballs, and ensure it's properly tracked in downstream metadata. Having bundled sources in multiple locations is going to make this very difficult, especially for people unfamiliar with WebKit. Example problem that has already occurred: it's accidentally included in the WebKitGTK 2.39.1 release tarballs. Had I not noticed it due to luck, it would have resulted in violation of security policy for downstream distributions that requires metadata for tracking bundled libraries. There's no way that product security organizations can properly react to a libavif vulnerability without metadata to know that it's there, but we didn't announce that we put it there, because how to know about it when it's in a random place? Whereas if it were under Source/ThirdParty, then it would have been automatically excluded from releases unless a human decided to allowlist it. Also, we can periodically check to see that everything there is kept is reasonably up to date.

So we should move libavif to Source/ThirdParty. Unfortunately, there's not much I can do to help with fixing this. Since this requires editing XCode build files, this move really has to be handled by Apple developers or someone familiar with XCode development.

Comment 1 Michael Catanzaro 2022-11-21 12:07:41 PST

Um, same problem with dav1d, except that one is even tricker since it's layered underneath libavif with no indication that it's yet another upstream source. I would have missed that entirely if not for the title of bug #246598.

Comment 2 Michael Catanzaro 2022-11-23 09:45:30 PST

A ideal layout for this would be: Source/ThirdParty/libavif, Source/ThirdParty/dav1d.

Alternatively, if keeping dav1d underneath the libavif directory is really desired, then it should be named differently to make clear that it is another third-party source, e.g. Source/ThirdParty/libavif/ThirdParty/dav1d.

Comment 3 Radar WebKit Bug Importer 2022-11-28 12:05:19 PST

<rdar://problem/102727980>

Comment 4 Myles C. Maxfield 2023-01-04 12:36:29 PST

We intentionally chose to put it inside WebCore because putting it outside WebCore would make it very difficult to integrate with Apple's build system.

The idea is that we would stop bundling it (and delete the checked-in copy!) in the future when all of Apple's OSes that ToT WebKit runs on would support avif natively. So we thought that all the pain of integrating with Apple's build system wouldn't be worth it for just a few years.

How do you feel about putting a few years' deadline on the amount of time that third party sources are in multiple directories to keep track of?

Comment 5 Michael Catanzaro 2023-01-04 13:19:26 PST

(In reply to Myles C. Maxfield from comment #4)
> We intentionally chose to put it inside WebCore because putting it outside
> WebCore would make it very difficult to integrate with Apple's build system.

This is strange because WebCore already has an unavoidable hard dependency on Source/ThirdParty due to both ANGLE and libwebrtc. But anyway, if it really needs to be under WebCore, how about Source/WebCore/ThirdParty?

Either:

 Source/WebCore/ThirdParty/libavif
 Source/WebCore/ThirdParty/dav1d

Or:

 Source/WebCore/ThirdParty/libavif
 Source/WebCore/ThirdParty/libavif/ThirdParty/dav1d

(Notably, I suggest avoiding Source/WebCore/ThirdParty/libavif/dav1d, to make it clear that's a second third-party source.)

If one of those suggestions works for you, then we can dodge the problem and not bother with discussing how long it's OK for things to be messy. :)

Comment 6 Myles C. Maxfield 2023-01-04 21:32:45 PST

Can we keep it in PAL?

Comment 7 Tim Nguyen (:ntim) 2023-01-04 21:53:02 PST

> We intentionally chose to put it inside WebCore because putting it outside WebCore would make it very difficult to integrate with Apple's build system.

PDF.js is prior art of integrating a ThirdParty directory into WebCore for XCode build purposes (see bug 241117)

Comment 8 Michael Catanzaro 2023-01-05 07:27:00 PST

(In reply to Myles C. Maxfield from comment #6)
> Can we keep it in PAL?

Seems like an odd place, but sure:

 Source/WebCore/PAL/ThirdParty/libavif
 Source/WebCore/PAL/ThirdParty/dav1d

Or:

 Source/WebCore/PAL/ThirdParty/libavif
 Source/WebCore/PAL/ThirdParty/libavif/ThirdParty/dav1d

Just stuffing ThirdParty into the path will help a lot.

Comment 9 Myles C. Maxfield 2023-01-07 17:26:11 PST

I'm working on this tonight.

Comment 10 Myles C. Maxfield 2023-01-07 18:02:13 PST

Okay, I have a patch that works with local builds. Let's make sure it builds with Apple's build system before uploading.

Comment 11 Myles C. Maxfield 2023-01-07 19:20:00 PST

https://github.com/WebKit/WebKit/pull/8364

Comment 12 Myles C. Maxfield 2023-01-07 21:02:26 PST

https://github.com/WebKit/WebKit/pull/8368

Comment 13 EWS 2023-01-08 15:30:16 PST

Committed 258646@main (8a6b8098b05c): <https://commits.webkit.org/258646@main>

Reviewed commits have been landed. Closing PR #8368 and removing active labels.