Add ability to sniff media content should we failed to play a file or if the provided content type was invalid. #25883
Conversation
|
EWS run on previous version of this PR (hash e8232c5)
|
jyavenard
changed the title
|
EWS run on previous version of this PR (hash 4db1621) |
webkit-ews-buildbot
added
the
merging-blocked
jyavenard
removed
the
merging-blocked
jyavenard
force-pushed
the
mimeSniffer
branch
2 times, most recently
from
b815695
to
e11bd30
Compare
|
EWS run on previous version of this PR (hash e11bd30) |
|
EWS run on previous version of this PR (hash b815695) |
webkit-ews-buildbot
added
the
merging-blocked
jyavenard
removed
the
merging-blocked
|
EWS run on previous version of this PR (hash cab8f9d) |
webkit-ews-buildbot
added
the
merging-blocked
jyavenard
removed
the
merging-blocked
|
EWS run on previous version of this PR (hash 935f528) |
jyavenard
force-pushed
the
mimeSniffer
branch
2 times, most recently
from
1802bb5
to
37b6e34
Compare
|
EWS run on previous version of this PR (hash 37b6e34) |
|
EWS run on previous version of this PR (hash 1802bb5) |
| : public PlatformMediaResourceClient { | ||
| WTF_MAKE_FAST_ALLOCATED; | ||
| public: | ||
| static RefPtr<MediaResourceSniffer> create(PlatformMediaResourceLoader&, ResourceRequest&&, size_t maxSize = -1); |
There was a problem hiding this comment.
Nit: consider an std::optional<size_t> here, rather than a -1, or (ref: Analyzing-Build-Performance), use overloading here to avoid the default parameter.
|
|
||
| mediaPlayerRenderingModeChanged(); | ||
| void HTMLMediaElement::sniffForContentType(const URL& url, Function<bool(const ContentType&)>&& completionHandler) |
There was a problem hiding this comment.
I'm curious why this takes a Function rather than returning a NativePromise?
There was a problem hiding this comment.
it just made it easier in this context, the function may not be called : sniffForContentType already checks for the WeakPtr and that the sniffing operation hasn't been cancelled. I would have had to run those checks for every caller otherwise.
it also avoids an extra dispatch to stay closer to the existing implementation.
There was a problem hiding this comment.
It is weird to have an async function that takes in a completion handler not consistently call its completion handler. I think this should take an actual WTF::CompletionHandler type is should be called consistently. It's really not a great API otherwise.
| if (!m_producer) | ||
| m_producer.resolve(WTFMove(mimeType)); |
There was a problem hiding this comment.
Ditto re: inverted condition.
There was a problem hiding this comment.
I added a Producer::isSettled() and will use that instead, it is indeed more explicit.
| if (!m_producer) | ||
| m_producer.reject(PlatformMediaError::Cancelled); |
There was a problem hiding this comment.
Nit: This seems like an inverted condition. :)
Apparently operator bool for a Producer returns whether the contained Promise is settled, but this looks super weird, because it implies that m_producer is a pointer (which it kind of is because it also has a operator ->.
This could be done in another patch, but I think Producer's operator bool should be replaced with an explicit isSettled().
There was a problem hiding this comment.
Producer::operator bool() is equivalent to promise::isSettled() ; I could is isNothing() instead.
|
|
||
| void MediaResourceSniffer::dataReceived(PlatformMediaResource&, const SharedBuffer& buffer) | ||
| { | ||
| auto mimeType = MIMESniffer::getMIMETypeFromContent({ buffer.data(), buffer.size() }); |
There was a problem hiding this comment.
This seems incorrect; it would only work on the first data that's received. Shouldn't the m_content.append(buffer) step happen first, and then the sniff?
There was a problem hiding this comment.
good point, I believe that in practice it will always work on the first packet received, as it's unlikely we will receive less than the 1485 bytes we requested.
There was a problem hiding this comment.
{ buffer.data(), buffer.size() } -> buffer.dataAsSpanForContiguousData(). It is a horrible name so I intend to rename soon but still.
| namespace WebCore { | ||
|
|
||
| class ContentType { | ||
| public: | ||
| WEBCORE_EXPORT explicit ContentType(String&& type); | ||
| WEBCORE_EXPORT explicit ContentType(const String& type); | ||
| WEBCORE_EXPORT ContentType(const String& type, const WTF::URL&); | ||
| WEBCORE_EXPORT ContentType(const String& type, bool); |
There was a problem hiding this comment.
Is this second constructor really needed? I don't see it used anywhere.
There was a problem hiding this comment.
it's used by the IPC serialiser. It is very weird how the serialiser works IMHO. I'll add a comment
There was a problem hiding this comment.
You could make it private and mark the serializer class as friend like we often do.
There was a problem hiding this comment.
I now use it following the move to have a fromURL() method
| namespace WebCore { | ||
|
|
||
| class ContentType { | ||
| public: | ||
| WEBCORE_EXPORT explicit ContentType(String&& type); | ||
| WEBCORE_EXPORT explicit ContentType(const String& type); | ||
| WEBCORE_EXPORT ContentType(const String& type, const WTF::URL&); |
There was a problem hiding this comment.
Since the URL isn't stored, I think this makes more sense as a static method; something like:
WEBCORE_EXPORT static fromURL(const WTF::URL&);
And the call site would change from:
// Attempt to determine the content-type from the URL file extension.
contentType = ContentType { emptyString(), url };
To:
// Attempt to determine the content-type from the URL file extension.
contentType = ContentType::fromURL(url);
| while (bytesRead < boxSize) { | ||
| // If the three bytes from sequence[bytes-read] to sequence[bytes-read + 2] are equal to 0x6D 0x70 0x34 ("mp4"), return true. | ||
| if (sequence[bytesRead] == 0x6d && sequence[bytesRead + 1] == 0x70 && sequence[bytesRead + 2] == 0x34) | ||
| return true; | ||
| // Increment bytes-read by 4. | ||
| bytesRead += 4; | ||
| } |
There was a problem hiding this comment.
Can we leverage ISOBox::peekBox() here?
There was a problem hiding this comment.
I wasn't aware of that method, however I really wanted us to be 100% spec compliant too and considering all the steps on how to read the box are defined in the spec; I feel it's better to follow it there.
|
EWS run on previous version of this PR (hash fe4fddc) |
webkit-ews-buildbot
added
the
merging-blocked
jyavenard
removed
the
merging-blocked
|
EWS run on previous version of this PR (hash 29530a6) |
|
EWS run on previous version of this PR (hash f036ce2) |
jyavenard
changed the title
|
EWS run on previous version of this PR (hash 71c6258) |
webkit-ews-buildbot
added
the
merging-blocked
jyavenard
removed
the
merging-blocked
jyavenard
changed the title
|
EWS run on previous version of this PR (hash 238ec12) |
webkit-ews-buildbot
added
the
merging-blocked
|
failures in Mac-wk1 are unrelated |
jyavenard
removed
the
merging-blocked
|
EWS run on previous version of this PR (hash 65bfc98) |
jyavenard
added
merge-queue
jyavenard
changed the title
|
EWS run on current version of this PR (hash 85db953) |
… if the provided content type was invalid. https://bugs.webkit.org/show_bug.cgi?id=270975 rdar://problem/124614908 Reviewed by Jer Noble. We have always relied on the server to provide a valid content-type and if that failed we always used the URL's file name extension instead. While the HTML5 specs clearly states that determining the type of a resource should be done through sniffing, this is however a too significant change to enable right away. There are also performance advantages in using the provided content-type: it's typically immediately available. Should the provided content-type be invalid or non-existent (such as with some blobs) playback would have failed. We add a MIMEtype sniffer for media content as per HTML5 specs. Should we fail to play a media using the older content type detection rather than immediately fail, we will also sniff the type as a last attempt before retrying. In order to not unnecessarily retry following a sniffing, we needed to distinguish a format error from a network error which the MediaPlayerPrivateAVFoundationObjC didn't do. As a fly-by fix, we add a way to distinguish the two errors by recording in the WebCoreNSURLSession any network failures. For now will limit sniffing to media element where the src attribute is set (and so exclude element where alternative sources are defined) Added API tests for the mimetype sniffer and tests for webm and mp4 in a blob. * LayoutTests/media/video-src-mp4-blob-expected.txt: Added. * LayoutTests/media/video-src-mp4-blob.html: Added. * LayoutTests/media/video-src-webm-blob-expected.txt: Added. * LayoutTests/media/video-src-webm-blob.html: Added. * LayoutTests/media/video-srcobject-mp4-blob-expected.txt: Added. * LayoutTests/media/video-srcobject-mp4-blob.html: Added. * LayoutTests/http/tests/media/video-throttled-load-metadata-expected.txt: * LayoutTests/http/tests/media/video-throttled-load-metadata.html: The test was racy and could caused the loadedmetadata message to have been received before the worker's message. * LayoutTests/platform/mac-wk1/TestExpectations: * LayoutTests/platform/mac/TestExpectations: Remove failure expectations. Tests were failing as an incorrect content-type was provided. Now that we sniff if the provided content-type was incorrect files can properly play again. * Source/WTF/wtf/NativePromise.h: * Source/WebCore/Headers.cmake: * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/html/HTMLMediaElement.cpp: Move all the logic from MediaPlayer related to guessing the ContentType if missing into the HTMLMediaElement. If all have failed, we will sniff the content. (WebCore::HTMLMediaElement::~HTMLMediaElement): (WebCore::HTMLMediaElement::loadResource): If the content is blocked, signal it as a network error rather than content error. Otherwise we will attempt to load the content twice (once for sniffing and once for playing) causing test failures. (WebCore::HTMLMediaElement::needsContentTypeToPlay const): Add methods to determine if sniffing is ultimately required. (WebCore::HTMLMediaElement::sniffForContentType): (WebCore::HTMLMediaElement::mediaLoadingFailed): Add one ultimate attempt if we failed to decode and didn't encounter a network error and didn't attempt to previously sniff content. (WebCore::HTMLMediaElement::cancelPendingTasks): (WebCore::HTMLMediaElement::cancelSniffer): (WebCore::HTMLMediaElement::mediaPlayerEngineFailedToLoad): Save if a network error was encountered. If so, we won't attempt to sniff later on. This is required as multiple tests are relying on the network being accessed only once, doing one extra access for sniffing made the tests fail. (WebCore::HTMLMediaElement::mediaPlayerEngineFailedToLoad const): Deleted. * Source/WebCore/html/HTMLMediaElement.h: * Source/WebCore/platform/CommonAtomStrings.h: * Source/WebCore/platform/ContentType.cpp: (WebCore::ContentType::ContentType): (WebCore::ContentType::fromURL): * Source/WebCore/platform/ContentType.h: (WebCore::ContentType::typeWasInferredFromExtension const): * Source/WebCore/platform/PlatformMediaError.cpp: (WebCore::convertEnumerationToString): * Source/WebCore/platform/PlatformMediaError.h: * Source/WebCore/platform/graphics/MIMESniffer.cpp: Added. (WebCore::MIMESniffer::span8): (WebCore::MIMESniffer::hasSignatureForMP4): (WebCore::MIMESniffer::parseWebMVint): (WebCore::MIMESniffer::hasSignatureForWebM): (WebCore::MIMESniffer::matchMP3Header): (WebCore::MIMESniffer::mp3FrameSize): (WebCore::MIMESniffer::parseMP3Frame): (WebCore::MIMESniffer::hasSignatureForMP3WithoutID3): (WebCore::MIMESniffer::mimeTypeFromSnifferEntries): (WebCore::MIMESniffer::getMIMETypeFromContent): * Source/WebCore/platform/graphics/MIMESniffer.h: Added. * Source/WebCore/platform/graphics/MediaPlayer.cpp: (WebCore::applicationOctetStream): (WebCore::MediaPlayer::load): (WebCore::MediaPlayer::loadWithNextMediaEngine): * Source/WebCore/platform/graphics/MediaPlayer.h: (WebCore::MediaPlayerClient::mediaPlayerEngineFailedToLoad): (WebCore::MediaPlayerClient::mediaPlayerEngineFailedToLoad const): Deleted. * Source/WebCore/platform/graphics/MediaResourceSniffer.cpp: Added. (WebCore::MediaResourceSniffer::create): (WebCore::MediaResourceSniffer::MediaResourceSniffer): (WebCore::MediaResourceSniffer::~MediaResourceSniffer): (WebCore::MediaResourceSniffer::cancel): (WebCore::MediaResourceSniffer::promise const): (WebCore::MediaResourceSniffer::dataReceived): (WebCore::MediaResourceSniffer::loadFailed): (WebCore::MediaResourceSniffer::loadFinished): * Source/WebCore/platform/graphics/MediaResourceSniffer.h: Added. * Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp: (WebCore::MediaPlayerPrivateAVFoundation::updateStates): We needed to be able to distinguish FormatError vs NetworkError so that we don't unnecessarily retry playback following sniffing. * Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h: * Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm: (WebCore::MediaPlayerPrivateAVFoundationObjC::assetStatus const): Distinguish load failure due to decoding error vs networking error. * Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp: (WebKit::RemoteMediaPlayerProxy::mediaPlayerEngineFailedToLoad): (WebKit::RemoteMediaPlayerProxy::mediaPlayerEngineFailedToLoad const): Deleted. * Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h: * Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in: * Tools/TestWebKitAPI/CMakeLists.txt: * Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj: * Tools/TestWebKitAPI/Tests/WebCore/MIMESniffer.cpp: Added. (TestWebKitAPI::TEST): Canonical link: https://commits.webkit.org/276258@main
webkit-commit-queue
force-pushed
the
mimeSniffer
branch
from
85db953
to
93f6303
Compare
|
Committed 276258@main (93f6303): https://commits.webkit.org/276258@main Reviewed commits have been landed. Closing PR #25883 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
93f6303
85db953
🧪 ios-wk2-wpt🛠 🧪 jsc-arm64