Handle case where decoding of image lead to an empty one.
rdar://77251937
Created attachment 427327 [details] Patch
Created attachment 427328 [details] Patch
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.
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.
Created attachment 427393 [details] Patch
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].
(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?
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.
(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.