Bug 225183 – Handle case where decoding of image lead to an empty one.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jean-Yves Avenard [:jya]]

Handle case where decoding of image lead to an empty one.

[Jean-Yves Avenard [:jya]]

rdar://77251937

[Jean-Yves Avenard [:jya]]

Created attachment 427327 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 427328 [details]
Patch

[Eric Carlson]

Comment on attachment 427328 [details]
Patch

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

> Source/WebCore/Modules/mediasession/MediaMetadata.cpp:184
> +            // Empty image; ignore.

Nit: I don't think this comment is necessary.

> Source/WebCore/html/MediaElementSession.cpp:1160
> +            ASSERT(sessionMetadata->artworkImage()->data());
>              artwork = NowPlayingInfoArtwork { sessionMetadata->artworkSrc(), sessionMetadata->artworkImage()->mimeType(), sessionMetadata->artworkImage()->data() };

Is there any reason to create the artwork if `sessionMetadata->artworkImage()->data()` is NULL?

> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:314
> +        ASSERT(nowPlayingInfo.artwork->imageData);
>          auto nsArtwork = nowPlayingInfo.artwork->imageData->createNSData();

This will crash in a release build, so either the assert isn't necessary or we should not try to create the artwork if the image data is NULL.

[Jean-Yves Avenard [:jya]]

Comment on attachment 427328 [details]
Patch

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

>> Source/WebCore/html/MediaElementSession.cpp:1160
>>              artwork = NowPlayingInfoArtwork { sessionMetadata->artworkSrc(), sessionMetadata->artworkImage()->mimeType(), sessionMetadata->artworkImage()->data() };
> 
> Is there any reason to create the artwork if `sessionMetadata->artworkImage()->data()` is NULL?

This is the point of the assert. An Image received here *must* have a SharedBuffer attached. It is an error if there's not.

This is the core of the change.

But Jer suggested I use a Ref rather than a RefPtr in NowPlayingInfoArtwork structure, however the code relies on the ability to set the value to a null pointer to indicate that the cached image should be used instead. So it would need to be made an Optional<Ref<SharedBuffer>> which is pretty much what a RefPtr is.

[Jean-Yves Avenard [:jya]]

Created attachment 427393 [details]
Patch

[EWS]

Committed r276827 (237182@main): <https://commits.webkit.org/237182@main>

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

[Eric Carlson]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> Comment on attachment 427328 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427328&action=review
> 
> >> Source/WebCore/html/MediaElementSession.cpp:1160
> >>              artwork = NowPlayingInfoArtwork { sessionMetadata->artworkSrc(), sessionMetadata->artworkImage()->mimeType(), sessionMetadata->artworkImage()->data() };
> > 
> > Is there any reason to create the artwork if `sessionMetadata->artworkImage()->data()` is NULL?
> 
> This is the point of the assert. An Image received here *must* have a
> SharedBuffer attached. It is an error if there's not.
> 
> This is the core of the change.

Right. My point was that NowPlayingInfoArtwork without image data is useless, ASSERTing in a debug build is good but why create the NowPlayingInfoArtwork in a release build?

[Jean-Yves Avenard [:jya]]

Comment on attachment 427328 [details]
Patch

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

>>>> Source/WebCore/html/MediaElementSession.cpp:1160
>>>>              artwork = NowPlayingInfoArtwork { sessionMetadata->artworkSrc(), sessionMetadata->artworkImage()->mimeType(), sessionMetadata->artworkImage()->data() };
>>> 
>>> Is there any reason to create the artwork if `sessionMetadata->artworkImage()->data()` is NULL?
>> 
>> This is the point of the assert. An Image received here *must* have a SharedBuffer attached. It is an error if there's not.
>> 
>> This is the core of the change.
>> 
>> But Jer suggested I use a Ref rather than a RefPtr in NowPlayingInfoArtwork structure, however the code relies on the ability to set the value to a null pointer to indicate that the cached image should be used instead. So it would need to be made an Optional<Ref<SharedBuffer>> which is pretty much what a RefPtr is.
> 
> Right. My point was that NowPlayingInfoArtwork without image data is useless, ASSERTing in a debug build is good but why create the NowPlayingInfoArtwork in a release build?

And it won’t be. As artwork will not be set if the image as no data following this change.

[Eric Carlson]

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> > Right. My point was that NowPlayingInfoArtwork without image data is useless, ASSERTing in a debug build is good but why create the NowPlayingInfoArtwork in a release build?
> 
> And it won’t be. As artwork will not be set if the image as no data
> following this change.

Sorry, I am not being very clear.

I was trying to say that there is no need to create the `Optional<NowPlayingInfoArtwork>` if we don't have image data, because that means we will also serialize and then deserialize it only to find out we can't use it.