Bug 201401

Summary: [GStreamer] cnn.com videos don't work
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: MediaAssignee: Alicia Boya GarcĂ­a <aboya>
Status: RESOLVED FIXED    
Severity: Normal CC: aboya, bugs-noreply, calvaris, cgarcia, commit-queue, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, mario, mcatanzaro, menard, ntim, philipj, pnormand, sergio, vjaquez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
See Also: https://github.com/web-platform-tests/wpt/pull/19538
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Michael Catanzaro 2019-09-02 07:42:47 PDT
I can't play the videos on https://www.cnn.com/2019/08/30/us/st-louis-child-shooting-arrest/index.html in Tech Preview with 2.25.4.

There's two videos on this page: "At least 12 children have been killed by gunfire in St. Louis since April" and then "Kids remember friends lost to gun violence in St. Louis". Both videos display only a black window, even though we have H.264 support in Tech Preview. The second video sometimes plays a couple seconds of audio, but not always. In both cases, the duration of the video  -- the second number -- changes, which should not happen. After a few seconds, which seems to vary, the first number (the progress) stops changing as well.
Comment 1 Philippe Normand 2019-09-03 02:36:57 PDT
I can reproduce on trunk / MiniBrowser. Going back to the revision before the MSE source element rewrite fixes the issue. So I'm afraid this could be a regression introduced in r249205
Comment 2 Philippe Normand 2019-09-03 02:45:37 PDT
The video track is not enabled. Commenting out the early return in MediaPlayerPrivateGStreamer::enableTrack() line 814, I now see video ;)
Comment 3 Michael Catanzaro 2019-09-03 04:05:47 PDT
But we have 2.25.4 in Tech Preview, and 2.25.4 does not have the MSE source element rewrite. So that cannot be the cause of the bug I'm experiencing. There must be something else wrong too. Right?
Comment 4 Philippe Normand 2019-09-03 04:19:11 PDT
Oh yes, this is a different issue indeed. Here in Ephy TP I have *NO* H.264 decoder:

[📦 org.gnome.Epiphany.Devel ~]$ gst-play-1.0 --videosink "glsinkbin sink=gtkglsink" "https://cnnios-f.akamaihd.net/i/cnn/big/us/2019/08/14/caption/7-year-old-killed-in-st-louis-gunfire-lc-orig.cnn_2782230_ios_,440,650,840,1240,3000,5500,.mp4.csmil/master.m3u8?__b__=650"
Press 'k' to see a list of keyboard shortcuts.
Now playing https://cnnios-f.akamaihd.net/i/cnn/big/us/2019/08/14/caption/7-year-old-killed-in-st-louis-gunfire-lc-orig.cnn_2782230_ios_,440,650,840,1240,3000,5500,.mp4.csmil/master.m3u8?__b__=650
WARNING No decoder available for type 'video/x-h264, stream-format=(string)byte-stream, alignment=(string)nal, parsed=(boolean)true'.
WARNING debug information: ../gst/playback/gsturidecodebin.c(920): unknown_type_cb (): /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0

And HLS is used, whereas in trunk MSE is used... What a mess.
Comment 5 Philippe Normand 2019-09-03 04:25:52 PDT
So to sum up, in TP 2.25.4, HLS is used but the runtime lost (again?) H.264 decoding support. That's not a WebKit bug.

However in trunk, MSE is used but only audio plays, no video. See comment 2.
Comment 6 Michael Catanzaro 2019-09-03 04:45:43 PDT
(In reply to Philippe Normand from comment #4)
> Oh yes, this is a different issue indeed. Here in Ephy TP I have *NO* H.264
> decoder:

lol....
Comment 7 Michael Catanzaro 2019-09-03 04:49:39 PDT
Let's repurpose this bug for the problem in trunk.

For losing H.264 again: https://gitlab.com/freedesktop-sdk/freedesktop-sdk/issues/858
Comment 8 Alicia Boya GarcĂ­a 2019-09-04 11:44:01 PDT
I got a local repro. The bug occurs 100% of the time when the audio initialization segment is appended before the video initialization segment, and 0% of the time when the order is the opposite.
Comment 9 Michael Catanzaro 2019-09-06 06:01:05 PDT
(In reply to Philippe Normand from comment #5)
> So to sum up, in TP 2.25.4, HLS is used but the runtime lost (again?) H.264
> decoding support. That's not a WebKit bug.

This is "fixed," now we support H.264 but any attempt to play it causes the web process to immediately crash:

https://gitlab.com/freedesktop-sdk/freedesktop-sdk/issues/863

I think help from WebKit multimedia experts will be needed to solve it.
Comment 10 Philippe Normand 2019-09-06 07:48:28 PDT
Seems like Mario is your expert, since he wrote(?) this "noopenh264" lib which I had no idea exists, until 5 minutes ago. :)
Comment 11 Michael Catanzaro 2019-09-06 08:00:01 PDT
It looks like freedesktop-sdk developers will deal with it.

It's a bit confusing discussing two different issues in the same place. I'll shut up and leave this one to Alicia.
Comment 12 Alicia Boya GarcĂ­a 2019-09-06 09:52:01 PDT
Yeah, that looks like a completely different issue. This one is about the video track not showing about half of the time.
Comment 13 Alicia Boya GarcĂ­a 2019-09-07 03:23:36 PDT
Created attachment 378288 [details]
Patch
Comment 14 Michael Catanzaro 2019-09-07 06:51:38 PDT
Comment on attachment 378288 [details]
Patch

Is it really not testable? Or it doesn't fix any existing layout tests?
Comment 15 Xabier RodrĂ­guez Calvar 2019-09-09 01:53:21 PDT
Comment on attachment 378288 [details]
Patch

A test is needed.
Comment 16 Alicia Boya GarcĂ­a 2019-09-10 11:21:12 PDT
As far as I know, this is not testable with LayoutTests. I tried using canvas to check the content of the video, but even when the video sink is using the wrong context and the displayed textures in the <video> are wrong, painting them on a canvas will download the correct ones, so testing this can't be automated in JS.
Comment 17 Xabier RodrĂ­guez Calvar 2019-09-11 00:47:42 PDT
(In reply to Alicia Boya GarcĂ­a from comment #16)
> As far as I know, this is not testable with LayoutTests. I tried using
> canvas to check the content of the video, but even when the video sink is
> using the wrong context and the displayed textures in the <video> are wrong,
> painting them on a canvas will download the correct ones, so testing this
> can't be automated in JS.

Isn't there any way with the internals?
Comment 18 Alicia Boya GarcĂ­a 2019-09-11 03:34:06 PDT
(In reply to Xabier RodrĂ­guez Calvar from comment #17)
> (In reply to Alicia Boya GarcĂ­a from comment #16)
> > As far as I know, this is not testable with LayoutTests. I tried using
> > canvas to check the content of the video, but even when the video sink is
> > using the wrong context and the displayed textures in the <video> are wrong,
> > painting them on a canvas will download the correct ones, so testing this
> > can't be automated in JS.
> 
> Isn't there any way with the internals?

Maybe it would work if I had a way to get a screenshot of the whole page (since getting the video alone doesn't reproduce the issue as I explained).

Reftests are supposed to do that, so that may be the way, if they happen to reproduce the bug. I'll give it a look.
Comment 19 Alicia Boya GarcĂ­a 2019-10-06 09:48:46 PDT
Created attachment 380299 [details]
Patch
Comment 20 Alicia Boya GarcĂ­a 2019-10-06 10:03:11 PDT
Created attachment 380300 [details]
Patch
Comment 21 Philippe Normand 2019-10-07 02:01:11 PDT
The test needs mac-specific expectation files it seems. You can get them from the EWS.
Comment 22 Alicia Boya GarcĂ­a 2019-10-07 05:11:25 PDT
Created attachment 380314 [details]
Patch
Comment 23 Alicia Boya GarcĂ­a 2019-10-07 10:18:06 PDT
Created attachment 380336 [details]
Patch
Comment 24 Philippe Normand 2019-10-09 01:31:29 PDT
There's a 2 pixel difference now in the mac test results. How come? :(
Comment 25 Alicia Boya GarcĂ­a 2019-10-09 04:24:12 PDT
(In reply to Philippe Normand from comment #24)
> There's a 2 pixel difference now in the mac test results. How come? :(

I have no idea. I can't even see it in an image editor using Difference blending mode.

And the difference is near the text, which is produced by identical HTML and CSS. I guess I'll just mark the test as skip in Mac, since I see no way to account for that couple of pixels.
Comment 26 Philippe Normand 2019-10-09 04:39:42 PDT
Go for it. We can't block this forever due to unwilling bots.
Comment 27 Alicia Boya GarcĂ­a 2019-10-09 07:59:47 PDT
Created attachment 380535 [details]
Patch
Comment 28 WebKit Commit Bot 2019-10-09 12:40:12 PDT
Comment on attachment 380535 [details]
Patch

Clearing flags on attachment: 380535

Committed r250922: <https://trac.webkit.org/changeset/250922>
Comment 29 WebKit Commit Bot 2019-10-09 12:40:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2019-10-09 12:41:17 PDT
<rdar://problem/56126329>
Comment 31 Alicia Boya GarcĂ­a 2019-10-24 07:40:51 PDT
This patch was almost mechanically reverted temporarily along the rest of the WebKitMediaSrc rework. In theory this should have been OK since this was supposed to fix a regression of that patch... But actually the bug was already causing problems before, the WebKitMediaSrc rework just exposed a more common case.

It has been reported that the same bug arise when adding some CSS styles to the <video> element, and the patch fixes the issue.

And the patch actually makes sense in any case... since without it some perfectly valid state transitions are not covered!

So I'm reintroducing it.
Comment 32 Alicia Boya GarcĂ­a 2019-10-24 08:05:45 PDT
Created attachment 381810 [details]
Patch
Comment 33 WebKit Commit Bot 2019-10-24 09:40:04 PDT
Comment on attachment 381810 [details]
Patch

Clearing flags on attachment: 381810

Committed r251544: <https://trac.webkit.org/changeset/251544>
Comment 34 WebKit Commit Bot 2019-10-24 09:40:06 PDT
All reviewed patches have been landed.  Closing bug.