Bug 189647

Summary:

[GStreamer] Add support for AV1 decoding

Product:

WebKit

Reporter:

Philippe Normand <pnormand>

Component:

Platform

Assignee:

Philippe Normand <pnormand>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

calvaris, commit-queue, ews-watchlist, rniwa, webkit-bug-importer, zan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews103 for mac-sierra	none
Archive of layout-test-results from ews104 for mac-sierra-wk2	none
Patch	none
Archive of layout-test-results from ews126 for ios-simulator-wk2	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Philippe Normand

Reported 2018-09-15 09:48:01 PDT

Attachments
Patch (42.70 KB, patch) 2018-09-15 10:25 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (42.71 KB, patch) 2018-09-15 10:29 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (43.53 KB, patch) 2018-09-16 03:03 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews103 for mac-sierra (2.29 MB, application/zip) 2018-09-16 04:07 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.96 MB, application/zip) 2018-09-16 04:19 PDT, EWS Watchlist	no flags	Details
Patch (43.52 KB, patch) 2018-09-16 04:36 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.26 MB, application/zip) 2018-09-16 06:35 PDT, EWS Watchlist	no flags	Details
Patch (44.37 KB, patch) 2018-09-16 07:33 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (41.38 KB, patch) 2018-09-17 02:41 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (41.47 KB, patch) 2018-09-17 03:49 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (41.48 KB, patch) 2018-09-18 06:28 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (41.41 KB, patch) 2018-09-18 06:37 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (41.92 KB, patch) 2018-09-18 09:40 PDT, Xabier Rodríguez Calvar	no flags	Details Formatted Diff Diff
Patch (41.91 KB, patch) 2018-09-18 09:56 PDT, Xabier Rodríguez Calvar	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Philippe Normand

Comment 1 2018-09-15 10:25:43 PDT

Created attachment 349855 [details] Patch

EWS Watchlist

Comment 2 2018-09-15 10:29:06 PDT

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.

Philippe Normand

Comment 3 2018-09-15 10:29:56 PDT

Created attachment 349856 [details] Patch

Philippe Normand

Comment 4 2018-09-16 03:03:48 PDT

Created attachment 349866 [details] Patch

EWS Watchlist

Comment 5 2018-09-16 04:07:54 PDT

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

EWS Watchlist

Comment 6 2018-09-16 04:07:56 PDT

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

EWS Watchlist

Comment 7 2018-09-16 04:19:15 PDT

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

EWS Watchlist

Comment 8 2018-09-16 04:19:17 PDT

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

Philippe Normand

Comment 9 2018-09-16 04:36:22 PDT

Created attachment 349869 [details] Patch

EWS Watchlist

Comment 10 2018-09-16 06:35:52 PDT

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

EWS Watchlist

Comment 11 2018-09-16 06:35:54 PDT

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

Philippe Normand

Comment 12 2018-09-16 07:33:28 PDT

Created attachment 349871 [details] Patch

Xabier Rodríguez Calvar

Comment 13 2018-09-16 23:57:47 PDT

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.

Philippe Normand

Comment 14 2018-09-17 02:41:55 PDT

Created attachment 349882 [details] Patch

Philippe Normand

Comment 15 2018-09-17 02:46:00 PDT

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...

Xabier Rodríguez Calvar

Comment 16 2018-09-17 03:31:49 PDT

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:)

Philippe Normand

Comment 17 2018-09-17 03:49:46 PDT

Created attachment 349883 [details] Patch

Xabier Rodríguez Calvar

Comment 18 2018-09-17 05:31:41 PDT

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.

Philippe Normand

Comment 19 2018-09-17 05:50:29 PDT

(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.

Xabier Rodríguez Calvar

Comment 20 2018-09-17 08:33:22 PDT

(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?

Philippe Normand

Comment 21 2018-09-17 08:43:22 PDT

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.

Xabier Rodríguez Calvar

Comment 22 2018-09-18 06:03:51 PDT

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?

Philippe Normand

Comment 23 2018-09-18 06:28:33 PDT

Created attachment 350017 [details] Patch

Philippe Normand

Comment 24 2018-09-18 06:37:02 PDT

Created attachment 350018 [details] Patch

Xabier Rodríguez Calvar

Comment 25 2018-09-18 09:40:47 PDT

Created attachment 350024 [details] Patch

Xabier Rodríguez Calvar

Comment 26 2018-09-18 09:43:27 PDT

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.

Xabier Rodríguez Calvar

Comment 27 2018-09-18 09:56:37 PDT

Created attachment 350027 [details] Patch

Xabier Rodríguez Calvar

Comment 28 2018-09-18 09:58:08 PDT

I give informal r+, please, get some review to finish it since I wrote part of it.

WebKit Commit Bot

Comment 29 2018-09-19 01:53:57 PDT

Comment on attachment 350027 [details] Patch Clearing flags on attachment: 350027 Committed r236165: <https://trac.webkit.org/changeset/236165>

WebKit Commit Bot

Comment 30 2018-09-19 01:53:59 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 31 2018-09-19 01:54:22 PDT

<rdar://problem/44595086>

Note You need to log in before you can comment on or make changes to this bug.