Bug 189647 – [GStreamer] Add support for AV1 decoding

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

.

[Philippe Normand]

Created attachment 349855 [details]
Patch

[EWS Watchlist]

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]

Created attachment 349856 [details]
Patch

[Philippe Normand]

Created attachment 349866 [details]
Patch

[EWS Watchlist]

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]

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

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]

Created attachment 349869 [details]
Patch

[EWS Watchlist]

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]

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]

Created attachment 349871 [details]
Patch

[Xabier Rodríguez Calvar]

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]

Created attachment 349882 [details]
Patch

[Philippe Normand]

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

Created attachment 349883 [details]
Patch

[Xabier Rodríguez Calvar]

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]

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

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

Created attachment 350017 [details]
Patch

[Philippe Normand]

Created attachment 350018 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 350024 [details]
Patch

[Xabier Rodríguez Calvar]

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]

Created attachment 350027 [details]
Patch

[Xabier Rodríguez Calvar]

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

[WebKit Commit Bot]

Comment on attachment 350027 [details]
Patch

Clearing flags on attachment: 350027

Committed r236165: <https://trac.webkit.org/changeset/236165>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44595086>