.
Created attachment 349855 [details] Patch
Attachment 349855 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2359: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 349856 [details] Patch
Created attachment 349866 [details] Patch
Comment on attachment 349866 [details] Patch Attachment 349866 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9234017 New failing tests: media/media-can-play-av1.html
Created attachment 349867 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 349866 [details] Patch Attachment 349866 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9234034 New failing tests: media/media-can-play-av1.html
Created attachment 349868 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 349869 [details] Patch
Comment on attachment 349869 [details] Patch Attachment 349869 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9234611 New failing tests: media/media-can-play-av1.html
Created attachment 349870 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 349871 [details] Patch
Comment on attachment 349871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349871&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2360 > + return av1DecoderFound ? MediaPlayer::IsSupported : MediaPlayer::MayBeSupported; Why do we return maybe if we find no suitable decoder? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-1372 > -MediaPlayer::SupportsType MediaPlayerPrivateGStreamerBase::extendedSupportsType(const MediaEngineSupportParameters& parameters, MediaPlayer::SupportsType result) > -{ > - UNUSED_PARAM(parameters); > - return result; > -} > - Though I understand it is pointless now, removing this makes me sad, we use it downstream and I hoped to use it upstream some day.
Created attachment 349882 [details] Patch
Comment on attachment 349871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349871&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2360 >> + return av1DecoderFound ? MediaPlayer::IsSupported : MediaPlayer::MayBeSupported; > > Why do we return maybe if we find no suitable decoder? Maybe because in theory the codec-installer could still kick in? Anyway for now I changed maybe to "", I don't have time to check the codec installer support. >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-1372 >> - > > Though I understand it is pointless now, removing this makes me sad, we use it downstream and I hoped to use it upstream some day. For now I've reverted this change as it's unrelated to this patch actually. The whole supportsType() code needs some yak-shaving across the 3 player classes...
Comment on attachment 349882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349882&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2364 > + return av1DecoderFound ? MediaPlayer::IsSupported : MediaPlayer::IsNotSupported; > + if (codec.startsWith("av1"_s)) > + return MediaPlayer::IsNotSupported; > + } > + return result; If we are still calling extendedSupportsType then we should not return here and let the other function filter as well O:)
Created attachment 349883 [details] Patch
Comment on attachment 349883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349883&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2364 > + return av1DecoderFound ? MediaPlayer::IsSupported : MediaPlayer::IsNotSupported; > + if (codec.startsWith("av1"_s)) > + return MediaPlayer::IsNotSupported; > + } > + return extendedSupportsType(parameters, result); Well, this is not what I meant, sorry for not being clear the first time. What I meant is that you should use "result", let execution continue till the end of the function and then call extendedSupportsType there. So remove these replace these three returns with result = when needed.
(In reply to Xabier Rodríguez Calvar from comment #18) > Comment on attachment 349883 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349883&action=review > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2364 > > + return av1DecoderFound ? MediaPlayer::IsSupported : MediaPlayer::IsNotSupported; > > + if (codec.startsWith("av1"_s)) > > + return MediaPlayer::IsNotSupported; > > + } > > + return extendedSupportsType(parameters, result); > > Well, this is not what I meant, sorry for not being clear the first time. > What I meant is that you should use "result", let execution continue till > the end of the function and then call extendedSupportsType there. So remove > these replace these three returns with result = when needed. But this doesn't make sense. The result variable will be changed below if we don't return early.
(In reply to Philippe Normand from comment #19) > But this doesn't make sense. The result variable will be changed below if we > don't return early. You can always change the next if to make it an else if, right?
Comment on attachment 349883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349883&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2369 > if (mimeTypeSet().contains(parameters.type.containerType())) Do you mean turn this specific `if` to an `else if`? No, this breaks existing canPlayType tests.
Comment on attachment 349883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349883&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2369 >> if (mimeTypeSet().contains(parameters.type.containerType())) > > Do you mean turn this specific `if` to an `else if`? No, this breaks existing canPlayType tests. Yes, I mean this. How can it break those tests?
Created attachment 350017 [details] Patch
Created attachment 350018 [details] Patch
Created attachment 350024 [details] Patch
Phil, after our discussion, I think I managed to fix the issues and regressions we talked about. Please, test this if you can. I won't be able to land it until tomorrow unless you want to watch it.
Created attachment 350027 [details] Patch
I give informal r+, please, get some review to finish it since I wrote part of it.
Comment on attachment 350027 [details] Patch Clearing flags on attachment: 350027 Committed r236165: <https://trac.webkit.org/changeset/236165>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44595086>