Bug 225368 – Streamline codec parsing, replacing uses of HashMap with SortedArrayMap

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Darin Adler]

Streamline codec parsing, replacing uses of HashMap with SortedArrayMap

[Darin Adler]

Created attachment 427706 [details]
Patch

[Darin Adler]

Created attachment 427725 [details]
Patch

[Darin Adler]

Created attachment 427727 [details]
Patch

[Darin Adler]

Passing all the EWS tests, so now looking for a reviewer.

[Sam Weinig]

Comment on attachment 427727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427727&action=review

> Source/WebCore/platform/graphics/HEVCUtilities.cpp:128
> +    return makeOptionalFromPointer(typesMap.tryGet(string));

Should we just make tryGet() return an Optional? Or, perhaps have a version of tryGet that does? This seems like it could be a pretty common idiom? (though, as always, this kind of change can and should be left until there is actual evidence that we need to have duplicate copies of makeOptionalFromPointer()).

> Source/WebCore/platform/graphics/cocoa/HEVCUtilitiesCocoa.mm:150
> +    Vector<uint16_t> vector;
> +    vector.reserveInitialCapacity(array.count);
> +    for (id value in array) {

I think you could use compactMap() here. If you don't want to (or it doesn't work), a shinkToFit() after the loop is probably a good idea here.

[Darin Adler]

Comment on attachment 427727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427727&action=review

>> Source/WebCore/platform/graphics/HEVCUtilities.cpp:128
>> +    return makeOptionalFromPointer(typesMap.tryGet(string));
> 
> Should we just make tryGet() return an Optional? Or, perhaps have a version of tryGet that does? This seems like it could be a pretty common idiom? (though, as always, this kind of change can and should be left until there is actual evidence that we need to have duplicate copies of makeOptionalFromPointer()).

I do keep running into the intersection between Optional and pointers; in a lot of cases the two are used almost interchangeably but they are not all that easy to convert between.

I can’t decide which is better for tryGet; the pointer lets us peek into the array, but the Optional has more clean object lifetime semantics for callers that don’t know about an underlying array as an implementation detail.

>> Source/WebCore/platform/graphics/cocoa/HEVCUtilitiesCocoa.mm:150
>> +    for (id value in array) {
> 
> I think you could use compactMap() here. If you don't want to (or it doesn't work), a shinkToFit() after the loop is probably a good idea here.

I don’t think either of those is right for this. This does not skip over invalid values, instead it fails and drops the vector if it encounters an invalid value. So the vector is always perfectly sized because of reserving the initial capacity and a shrink to fit is not needed.

[EWS]

Committed r277093 (237395@main): <https://commits.webkit.org/237395@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427727 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77609399>

[Sam Weinig]

(In reply to Darin Adler from comment #6)
> Comment on attachment 427727 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427727&action=review
> 
> 
> >> Source/WebCore/platform/graphics/cocoa/HEVCUtilitiesCocoa.mm:150
> >> +    for (id value in array) {
> > 
> > I think you could use compactMap() here. If you don't want to (or it doesn't work), a shinkToFit() after the loop is probably a good idea here.
> 
> I don’t think either of those is right for this. This does not skip over
> invalid values, instead it fails and drops the vector if it encounters an
> invalid value. So the vector is always perfectly sized because of reserving
> the initial capacity and a shrink to fit is not needed.


Oops. Indeed.