.
Created attachment 353693 [details] Patch
Comment on attachment 353693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353693&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:575 > + auto& gstRegistryScanner = GstRegistryScanner::singleton(); oops should use MSE scanner here. Will fix up
Created attachment 353695 [details] Patch
Created attachment 353700 [details] Patch
Comment on attachment 353700 [details] Patch As spoken with Phil in private, it looks like this patch has issues so it needs more work. Removing flag.
Created attachment 353707 [details] Patch
Comment on attachment 353707 [details] Patch webkit-patch screwed up
Created attachment 353714 [details] Patch
Created attachment 353838 [details] Patch
EWS should process the patch now. Please review!
Comment on attachment 353838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353838&action=review There are still some things, mainly style I'd like to discuss, but it's overall a very nice rework! Thx! > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:23 > +#include "GstRegistryScanner.h" This include should be just below config.h > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:53 > + GST_DEBUG("%s registry scanner initialized", m_isMediaSource ? "MSE":"non-MSE"); "MSE" : "Regular playback", mind the space around the semicolon > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:57 > + GST_DEBUG("%s codec pattern registered: %s", item.value ? "Hardware":"Software", item.key.string().utf8().data()); mind the space around the semicolon > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:70 > +GstRegistryScanner::RegistryLookupResult GstRegistryScanner::gstRegistryHasElementForMediaType(GList* elementFactories, const char* capsString, bool checkHardwareClassifier) shouldCheckHardwareClassifier > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:75 > + bool supported = candidates; > + bool usingHardware = false; isSupported isUsingHardware > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:274 > +bool GstRegistryScanner::supportsCodec(String codec, bool usingHardware) const isUsingHardware or from what I understand, shouldCheckForHardwareUse > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:285 > + return usingHardware ? item.value : true; An alternative way could be: return !usingHardware || item.value; > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:288 > + GST_LOG("Unsupported"); You're logging only if a codec is not supported and I think it would expect all checks to be logged. Maybe something like : ... bool supported { false }; for (...) { if (!fnmatch(...)) { // Note you don't need the variable supported = !usingHardware || item.value; break; } } GST_DEBUG("Checked %s codec \"%s\" supported %s", usingHardware ? "hardware" : "software", codec.utf8().data(), boolForPrinting(supported)); return supported; > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:292 > +bool GstRegistryScanner::supportsAllCodecs(const Vector<String>& codecs, bool usingHardware) const ditto > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:302 > +bool GstRegistryScanner::isDecodingSupported(MediaConfiguration& configuration, bool* usingHardware) const From what I see in this patch, we always check for hardware when calling this, so a better signature would be: RegistryLookupResult GstRegisterScanner::isDecodingSupported(MediaConfiguracion& configuration) const Either that our use std::pair<bool, bool>. Of course, please get rid of all checks for nullptr. > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:47 > + bool supportsCodec(String codec, bool usingHardware = false) const; > + bool supportsAllCodecs(const Vector<String>& codecs, bool usingHardware = false) const; isCodecSupported areAllCodecsSupported > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:70 > + bool usingHardware; Considering that there are some methods returning this that can answer without checking for hardware support (if requested in the function signature) I would type this as std::optional<bool> so that you can leave it as nullopt if you are not checking for it. Actually I don't kind of like the assumption of considering a bunch of codecs just as software because we don't check them, I think we should consider them as hardware support unknown. This is not a strong request, just a suggestion. If you still prefer to consider unknown hardware support as false, I would recommend to turn this into an enum like NotSupported, SoftwareSupported, HardwareSupported. > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:74 > + RegistryLookupResult gstRegistryHasElementForMediaType(GList* elementFactories, const char* capsString, bool checkHardwareClassifier = false); I think doesHaveElementForMediaType is a better name. I don't understand why you prefixed the class name here. > Source/WebCore/platform/graphics/gstreamer/MediaEngineConfigurationFactoryGStreamer.cpp:57 > + bool usingHardware = false; isUsingHardware > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2244 > + if (codecs.isEmpty()) > + result = MediaPlayer::MayBeSupported; > + else > + result = gstRegistryScanner.supportsAllCodecs(codecs) ? MediaPlayer::IsSupported : MediaPlayer::IsNotSupported; We can write it as result = codecs.isEmpty() ? ... : ... > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:167 > + if (type.endsWith("mp4") || type.endsWith("aac")) This seems to be new, out of refactor scope, right? why?
Comment on attachment 353838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353838&action=review >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:285 >> + return usingHardware ? item.value : true; > > An alternative way could be: return !usingHardware || item.value; How is it better? I think I prefer mine because it's more explicit :) >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:70 >> + bool usingHardware; > > Considering that there are some methods returning this that can answer without checking for hardware support (if requested in the function signature) I would type this as std::optional<bool> so that you can leave it as nullopt if you are not checking for it. > > Actually I don't kind of like the assumption of considering a bunch of codecs just as software because we don't check them, I think we should consider them as hardware support unknown. This is not a strong request, just a suggestion. > > If you still prefer to consider unknown hardware support as false, I would recommend to turn this into an enum like NotSupported, SoftwareSupported, HardwareSupported. I'm sorry, I don't see what's wrong with this code :) >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:74 >> + RegistryLookupResult gstRegistryHasElementForMediaType(GList* elementFactories, const char* capsString, bool checkHardwareClassifier = false); > > I think doesHaveElementForMediaType is a better name. I don't understand why you prefixed the class name here. As many things in this patch, this is the result of a refactoring. gstRegistryHasElementForMediaType was the name used for the previous implementation in GStreamerCommon. >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:167 >> + if (type.endsWith("mp4") || type.endsWith("aac")) > > This seems to be new, out of refactor scope, right? why? As documented in the ChangeLog: * platform/graphics/gstreamer/mse/AppendPipeline.cpp: Ditto. Also plug qtdemux for AAC containers, this is an explicit consequence of finer-grained codecs probing.
Created attachment 362011 [details] Patch
Comment on attachment 362011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362011&action=review I will answer your comments in another entry. > Source/WebCore/platform/GStreamer.cmake:31 > + platform/graphics/gstreamer/mse/GstRegistryScannerMSE.cpp I think we should call this GStreamerRegistryScanner. > Source/WebCore/platform/graphics/MediaPlayer.cpp:1621 > +String convertEnumerationToString(MediaPlayerEnums::SupportsType enumerationValue) You could most probably make this a const String&, but we could leave this for a follow up patch for this and the rest of overloads. > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:25 > +#include "GRefPtrGStreamer.h" I'd recommend GStreamerCommon.h instead. > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:73 > + bool isSupported = candidates; If I am not wrong you can move this below. > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:77 > + for (GList* factories = candidates; factories != nullptr; factories = g_list_next(factories)) { !factories > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:145 > + m_codecMap.add(AtomicString("mpeg"), false); > + m_codecMap.add(AtomicString("mp4a*"), false); Why are we always assuming that this does not have hardware support? Isn't it better to check it the result for hardware support? Same for opus and vorbis, speex, etc > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:278 > + codec = codec.substring(slashIndex+1); spaces around + > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:310 > + GST_DEBUG("Checking support for video configuration: \"%s\" size: %ux%u bitrate: %" G_GUINT64_FORMAT " framerate: %f", videoConfiguration.contentType.utf8().data(), videoConfiguration.width, videoConfiguration.height, videoConfiguration.bitrate, videoConfiguration.framerate); This line is too long > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:321 > + GST_DEBUG("Checking support for audio configuration: \"%s\" %s channels, bitrate: %" G_GUINT64_FORMAT " samplerate: %u", audioConfiguration.contentType.utf8().data(), audioConfiguration.channels.utf8().data(), audioConfiguration.bitrate, audioConfiguration.samplerate); ditto > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:43 > + bool supportsContainerType(String containerType) const { return m_mimeTypeSet.contains(containerType); } isContainerTypeSupported > Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:47 > + bool supported; > + bool usingHardware; isSupported and isUsingHardware.
(In reply to Philippe Normand from comment #12) > >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:285 > >> + return usingHardware ? item.value : true; > > > > An alternative way could be: return !usingHardware || item.value; > > How is it better? I think I prefer mine because it's more explicit :) Just a preference, your call. > >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:70 > >> + bool usingHardware; > > > > Considering that there are some methods returning this that can answer without checking for hardware support (if requested in the function signature) I would type this as std::optional<bool> so that you can leave it as nullopt if you are not checking for it. > > > > Actually I don't kind of like the assumption of considering a bunch of codecs just as software because we don't check them, I think we should consider them as hardware support unknown. This is not a strong request, just a suggestion. > > > > If you still prefer to consider unknown hardware support as false, I would recommend to turn this into an enum like NotSupported, SoftwareSupported, HardwareSupported. > > I'm sorry, I don't see what's wrong with this code :) What I see wrong here is two things. The first I already commented about inline in the new patch, for some codec you're not checking if they have hardware support, just assuming false. The second is that IMHO, I think you shouldn't assume false. IMHO you should have three values, Unknown, No and Yes. Another way of having three values is using WTF::Optional and considering nullopt as non-checked. If you still think having three values is not useful, my initial recommendation was to use an enum NotSupported, SoftwareSupported and HardwareSupported but now that I had a closed look to the result struct, I find it nicer than before so I won't tell you anything about this. Summing up, I don't understand why you don't always initialize hardware support and assume false in some cases or consider having three values if you don't always check for hardware. This is my opinion about the code, I won't object if you land it as it is now either. > >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:74 > >> + RegistryLookupResult gstRegistryHasElementForMediaType(GList* elementFactories, const char* capsString, bool checkHardwareClassifier = false); > > > > I think doesHaveElementForMediaType is a better name. I don't understand why you prefixed the class name here. > > As many things in this patch, this is the result of a refactoring. > gstRegistryHasElementForMediaType was the name used for the previous > implementation in GStreamerCommon. Yes, I know it is a refactoring and a nice one. But this being a refactoring is not a reason to unprefix a function when you move it inside a class with other unprefixed methods. Please change this. > >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:167 > >> + if (type.endsWith("mp4") || type.endsWith("aac")) > > > > This seems to be new, out of refactor scope, right? why? > > As documented in the ChangeLog: > * platform/graphics/gstreamer/mse/AppendPipeline.cpp: Ditto. Also > plug qtdemux for AAC containers, this is an explicit consequence > of finer-grained codecs probing. Ok.
I'm checking for hardware support only for video decoders because the gst element klass Hardware classifier is currently used only for video decoders and moreover, audio decoding usually doesn't rely on specific hardware anyway (please correct me if this is wrong)
Comment on attachment 362011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362011&action=review >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:73 >> + bool isSupported = candidates; > > If I am not wrong you can move this below. Just before the gst_plugin_feature_list_free() then? It doesn't make much difference, I also prefer to have both vars initialized at the same place. It's a nit anyway :) >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:77 >> + for (GList* factories = candidates; factories != nullptr; factories = g_list_next(factories)) { > > !factories s/!// :) >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:145 >> + m_codecMap.add(AtomicString("mp4a*"), false); > > Why are we always assuming that this does not have hardware support? Isn't it better to check it the result for hardware support? Same for opus and vorbis, speex, etc Because audio decoders don't require special hardware
(In reply to Philippe Normand from comment #16) > I'm checking for hardware support only for video decoders because the gst > element klass Hardware classifier is currently used only for video decoders > and moreover, audio decoding usually doesn't rely on specific hardware > anyway (please correct me if this is wrong) Never checked that but it you think so, feel free to keep it. (In reply to Philippe Normand from comment #17) > >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:73 > >> + bool isSupported = candidates; > > > > If I am not wrong you can move this below. > > Just before the gst_plugin_feature_list_free() then? It doesn't make much > difference, I also prefer to have both vars initialized at the same place. > It's a nit anyway :) WebKit coding style, right? > >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:77 > >> + for (GList* factories = candidates; factories != nullptr; factories = g_list_next(factories)) { > > > > !factories > > s/!// :) True! > >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:145 > >> + m_codecMap.add(AtomicString("mp4a*"), false); > > > > Why are we always assuming that this does not have hardware support? Isn't it better to check it the result for hardware support? Same for opus and vorbis, speex, etc > > Because audio decoders don't require special hardware
(In reply to Xabier Rodríguez Calvar from comment #18) > > (In reply to Philippe Normand from comment #17) > > >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:73 > > >> + bool isSupported = candidates; > > > > > > If I am not wrong you can move this below. > > > > Just before the gst_plugin_feature_list_free() then? It doesn't make much > > difference, I also prefer to have both vars initialized at the same place. > > It's a nit anyway :) > > WebKit coding style, right? > https://webkit.org/code-style-guidelines/ doesn't mention this. The style bot is green as well.
Committed r241585: <https://trac.webkit.org/changeset/241585>
<rdar://problem/48109225>
Comment on attachment 362011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362011&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:146 > + if (type.endsWith("mp4") || type.endsWith("aac")) audio/x-aac? Where have you found that?
Comment on attachment 362011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362011&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:146 >> + if (type.endsWith("mp4") || type.endsWith("aac")) > > audio/x-aac? Where have you found that? why are you talking about x-aac? IIRC I changed this because some MSE layout tests were failing after my changes.