Bug 239916 - [Mac] VTVideoDecoderClass object pointers can become unaligned on x86
Summary: [Mac] VTVideoDecoderClass object pointers can become unaligned on x86
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-29 16:02 PDT by Jer Noble
Modified: 2022-04-30 00:27 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.47 KB, patch)
2022-04-29 16:28 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2022-04-29 16:02:40 PDT
[Mac] VTVideoDecoderClass object pointers can become unaligned on x86
Comment 1 Jer Noble 2022-04-29 16:03:10 PDT
<rdar://92445366>
Comment 2 Jer Noble 2022-04-29 16:28:20 PDT
Created attachment 458617 [details]
Patch
Comment 3 Eric Carlson 2022-04-29 16:49:57 PDT
Comment on attachment 458617 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP8Decoder.cpp:61
> +struct DecoderBaseClass {
>      uint8_t pad[padSize];
>      CMBaseClass alignedClass;
>  };

Can we put this in WebKitDecoder.h so it can be shared by the decoders?

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP8Decoder.cpp:86
> +static_assert(offsetof(DecoderBaseClass, alignedClass) == padSize, "CMBaseClass offset is incorrect!");
> +static_assert(alignof(DecoderBaseClass) == 4, "CMBaseClass must have 4 byte alignment");

Ditto

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP8Decoder.cpp:95
> +#pragma pack(push, 4)
> +struct DecoderClass {
> +    uint8_t pad[padSize];
> +    VTVideoDecoderClass alignedClass;
> +};

Ditto

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP8Decoder.cpp:122
> +static_assert(offsetof(DecoderClass, alignedClass) == padSize, "CMBaseClass offset is incorrect!");
> +static_assert(alignof(DecoderClass) == 4, "CMBaseClass must have 4 byte alignment");

Ditto
Comment 4 Jer Noble 2022-04-29 18:08:42 PDT
(In reply to Eric Carlson from comment #3)
> Comment on attachment 458617 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458617&action=review
> 
> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitVP8Decoder.cpp:61
> > +struct DecoderBaseClass {
> >      uint8_t pad[padSize];
> >      CMBaseClass alignedClass;
> >  };
> 
> Can we put this in WebKitDecoder.h so it can be shared by the decoders?

We'd have to expose CMBaseClass in those headers as well, and that header is used outside of just the VP8 and VP9 implementations.
Comment 5 Alexey Proskuryakov 2022-04-29 18:38:44 PDT
Comment on attachment 458617 [details]
Patch

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

> Source/ThirdParty/libwebrtc/ChangeLog:11
> +        dereferencing that pointer will fail.

I know that this is a performance issue, but I don't think that it ever fails on Intel? Perhaps there is some software limitation between WebKit and the CPU though.
Comment 6 EWS 2022-04-30 00:27:11 PDT
Committed r293643 (250147@main): <https://commits.webkit.org/250147@main>

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