This video plays perfectly fine in WebKitGTK+ 2.23.3: https://ftp.osuosl.org/pub/fosdem/2019/H.1309/media_gstreamer_wpe.webm Except the media controls are missing! It should surely be displaying WebKit's media controls.
I can't reproduce this issue with MiniBrowser from ToT.
Indeed, it works in MiniBrowser, but it's broken in the GNOME runtime. When 2.23.90 is released we should check if it's fixed there.
(In reply to Michael Catanzaro from comment #2) > Indeed, it works in MiniBrowser, but it's broken in the GNOME runtime. When > 2.23.90 is released we should check if it's fixed there. Ping :)
Nope, still no media controls with 2.24.0.
Looks like an Ephy bug then. Before reopening please provide a reproducer with MiniBrowser.
(In reply to Philippe Normand from comment #5) > Looks like an Ephy bug then. Before reopening please provide a reproducer > with MiniBrowser. You know Epiphany doesn't have anything to do with media controls. The WebKit development jhbuild is old and stale, whereas the GNOME master runtime is shiny and new. No wonder the set of bugs is different.
I checked Fedora 30's Epiphany, with WebKitGTK 2.23.91. The media controls slider is working fine. This strongly suggests that it's not Epiphany's fault, but something else in the runtime. You should reopen this until it's fixed.
(In reply to Michael Catanzaro from comment #6) > (In reply to Philippe Normand from comment #5) > > Looks like an Ephy bug then. Before reopening please provide a reproducer > > with MiniBrowser. > > You know Epiphany doesn't have anything to do with media controls. > Of course I know that. But perhaps Ephy uses a different API to load URIs? > The WebKit development jhbuild is old and stale, whereas the GNOME master > runtime is shiny and new. No wonder the set of bugs is different. I've just updated my local WebKit-Flatpak to the 3.32 GNOME Platform/Sdk and wasn't able to reproduce the bug there either (with MiniBrowser).
Try the master SDK, that's what Ephy Tech Preview uses. It probably has newer GStreamer or something.
(In reply to Philippe Normand from comment #8) > Of course I know that. > But perhaps Ephy uses a different API to load URIs? webkit_web_view_load_uri()?
(In reply to Michael Catanzaro from comment #9) > Try the master SDK, that's what Ephy Tech Preview uses. It probably has > newer GStreamer or something. Controls are not a GStreamer thing.
(In reply to Michael Catanzaro from comment #7) > I checked Fedora 30's Epiphany, with WebKitGTK 2.23.91. The media controls > slider is working fine. This strongly suggests that it's not Epiphany's > fault, but something else in the runtime. You should reopen this until it's > fixed. OK, now one month later with WebKitGTK 2.24.0 it's broken. So I don't think WebKit version is to blame here. Whatever is to blame must have made it into Fedora within the past month. There are errors in the web inspector when playing Phil's video in the first comment: [Error] SyntaxError: Unexpected string literal 'v' [Error] Failed to load resource: Plug-in handled load (media_gstreamer_wpe.webm, line 0) (Plus an unrelated favicon error.) I think these errors are our hints.
(In reply to Michael Catanzaro from comment #12) > [Error] Failed to load resource: Plug-in handled load > (media_gstreamer_wpe.webm, line 0) This is coming from WebFrameLoaderClient::committedLoad and it seems to be intentional for any standalone media document, which is weird because of course intentional internal errors shouldn't reach the web inspector.
This one is really bad.
Is there a way to help fixing this issue?
The issue is that, for some reason, the shadow DOM (supposed to contain the media controls) of the <video> element is empty. You can check that with the web-inspector. But since this issue happens only with the Ephy flatpak runtime, it's quite hard to track down... And it's not only for MediaDocuments, it's for all <video> relying on WebKit's media controls.
(In reply to Philippe Normand from comment #16) > The issue is that, for some reason, the shadow DOM (supposed to contain the > media controls) of the <video> element is empty. You can check that with the > web-inspector. > > But since this issue happens only with the Ephy flatpak runtime, it's quite > hard to track down... > > And it's not only for MediaDocuments, it's for all <video> relying on > WebKit's media controls. And it's the same for <audio> too. I've reinstalled older versions of Epiphany (the oldest being 3.24.0) with latest WebKitGTK+, and the controls are always missing. I think that controls were visible at least 2 years ago, so I can imagine that the bug is not caused by a change in Epiphany. Bisecting WebKitGTK+ is really painful, but I can do this. If anyone knows where "SyntaxError: Unexpected string literal 'v'" comes from (and if it's related), that would be helpful!
Seems like that error comes from Source/JavaScriptCore/parser/Parser.cpp but I doubt it's related.
With Ephy 3.32.1.2 (wkgtk 2.24.3) from Debian Testing, I do see controls.
(In reply to Guilaume Ayoub from comment #17) > Bisecting WebKitGTK+ is really painful, but I can do this. Thanks so much. But I'm really not sure a WebKit change is to blame. You might need to do a flatpak-bisect to bisect the runtime itself, except this bug is now so old that there's probably not enough old runtimes left on the server, so it's probably impossible.
Ah sorry, I guess you're testing with Fedora packages. Anyway, first step before we start bisecting is to pin down which package change caused the problem. If we can verify that it's really caused by a WebKit change, then it's bisectable, but unfortunately I'm not confident that's what we'll find. :/
(In reply to Michael Catanzaro from comment #21) > Ah sorry, I guess you're testing with Fedora packages. I'm not testing with Fedora, I'm using Gentoo. Fedora's not to blame here :). > Anyway, first step > before we start bisecting is to pin down which package change caused the > problem. If we can verify that it's really caused by a WebKit change, then > it's bisectable, but unfortunately I'm not confident that's what we'll find. > :/ I can at least rebuild an old version of WebKit and see if the controls are visible. That way we can be sure that the problem is not caused by another package.
OK, thanks. Figuring out what package would be tremendously helpful.
Media controls are visible with Epiphany 2.28.0 + WebKitGTK 2.20.3. I continue the bisection, but it's long because Ephy and WebKitGTK versions can't be mixed as we want…
Media controls are visible with Epiphany 2.30.3 + WebKitGTK 2.22.3.
You mean Epiphany 3.28.0 and Epiphany 3.30.3? There's no need to build Epiphany at all because this bug occurs in MiniBrowser. On Fedora it's packaged as /usr/libexec/webkit2gtk-4.0/MiniBrowser. Probably on Gentoo it would be under /usr/lib/.
Media controls are also visible with Epiphany 2.31.91 + WebKitGTK 2.23.2. As the original bug report is for WebKitGTK 2.23.3, I'll compile this version and check that the media controls are missing. We'll be sure that the problem comes from a change between 2.23.2 and 2.23.3. It's time for me to sleep while my computer is compiling, see you tomorrow! (In reply to Michael Catanzaro from comment #26) > You mean Epiphany 3.28.0 and Epiphany 3.30.3? > > There's no need to build Epiphany at all because this bug occurs in > MiniBrowser. On Fedora it's packaged as > /usr/libexec/webkit2gtk-4.0/MiniBrowser. Probably on Gentoo it would be > under /usr/lib/. Oh, as it was said in Comment #2 that the bug wasn't visible in MiniBrowser, I assumed that I had to compile Epiphany too. Well, compiling Epiphany is so fast that it doesn't bother me…
I think this issue may be the same (or very similar) than bug 200894 Can you check in your CMakeCache.txt file for the build the value you have configured for PYTHON_EXECUTABLE? If its pointing to python3 please test to rebuild with python2. First install python2 (if needed) and then pass something like "-DPYTHON_EXECUTABLE=/usr/bin/python2.7". Test this from a clean build state.
(In reply to Carlos Alberto Lopez Perez from comment #28) > I think this issue may be the same (or very similar) than bug 200894 > > Can you check in your CMakeCache.txt file for the build the value you have > configured for PYTHON_EXECUTABLE? If its pointing to python3 please test to > rebuild with python2. First install python2 (if needed) and then pass > something like "-DPYTHON_EXECUTABLE=/usr/bin/python2.7". Test this from a > clean build state. You're probably right. I have controls with 2.23.3 compiled with Python 2.7, but I didn't have them with 2.24.x versions compiled with Python 3.7. Do you want some help finding the problem in Python scripts?
Carlos!!!!! You nailed the bug!
Guillaume, Maybe start with checking these 2 fellows? Source/WebCore/css/makeSelectorPseudoClassAndCompatibilityElementMap.py Source/WebCore/css/makeSelectorPseudoElementsMap.py
(In reply to Philippe Normand from comment #31) > Guillaume, > > Maybe start with checking these 2 fellows? > > Source/WebCore/css/makeSelectorPseudoClassAndCompatibilityElementMap.py > Source/WebCore/css/makeSelectorPseudoElementsMap.py They generate the same files with Py2 and Py3. I've also tried Source/JavaScriptCore/Scripts/inline-and-minify-stylesheets-and-scripts.py, it gives the same results.
We would need the build to fail when the python3 raises an exception, basically. I can have a look at this later on today.
(In reply to Philippe Normand from comment #33) > We would need the build to fail when the python3 raises an exception, > basically. I can have a look at this later on today. I'm not sure that it raises an exception, it may just generate a JS or CSS file with a syntax error.
Comparing 2 builds, Source/JavaScriptCore/Scripts/make-js-file-arrays.py generates broken UserAgentScriptsData.cpp for Python3 builds.
Starting a patch, the js minifier is broken as well...
Created attachment 376763 [details] Patch
Fixes #200894 as well \o/
(In reply to Philippe Normand from comment #37) > Created attachment 376763 [details] > Patch It works perfectly, thanks a lot everybody! <3
Comment on attachment 376763 [details] Patch It seems to work, but I'm a bit puzzled why it still gives different results for python2 and python3. See: $ python2 Source/JavaScriptCore/Scripts/make-js-file-arrays.py test2.h test2.cpp -n WebCore Source/WebCore/Modules/mediacontrols/mediaControlsBase.js $ python3 Source/JavaScriptCore/Scripts/make-js-file-arrays.py test3.h test3.cpp -n WebCore Source/WebCore/Modules/mediacontrols/mediaControlsBase.js $ diff -u test2.h test3.h --- test2.h 2019-08-20 13:02:55.818755324 +0000 +++ test3.h 2019-08-20 13:03:06.312126092 +0000 @@ -1,3 +1,3 @@ namespace WebCore { -extern const char mediaControlsBaseJavaScript[35332]; +extern const char mediaControlsBaseJavaScript[46873]; } // namespace WebCore $ diff -u test2.cpp test3.cpp [... redacted, lots of diffs ... ] Any idea what causes this?
*** Bug 200894 has been marked as a duplicate of this bug. ***
The biggest issue seems to be in the minifier, the bug was introduced during its port to Python3 I think: > /home/phil/WebKit/Source/JavaScriptCore/Scripts/jsmin.py(89)write() -> self.outs.write(str(char)) (Pdb) char b'v' (Pdb) str(char) "b'v'" (Pdb) the write function declared line 79 doesn't handle byte values, you can see the b is embedded in the string. This is where the SyntaxError comes from at runtime.
Though that doesn't explain why the array sizes differ... It's a bit odd :)
(In reply to Philippe Normand from comment #43) > Though that doesn't explain why the array sizes differ... It's a bit odd :) Ah found it. In that same write function, for Python3 the return_buf should be a byte string, and the 'return' constant as well.
Comment on attachment 376763 [details] Patch This needs some more investigation...
(In reply to Philippe Normand from comment #45) > Comment on attachment 376763 [details] > Patch > > This needs some more investigation... By printing the generated char array to stdout with a small C program I can see that in the case of python3 the JS is not minified.
(In reply to Carlos Alberto Lopez Perez from comment #46) > (In reply to Philippe Normand from comment #45) > > Comment on attachment 376763 [details] > > Patch > > > > This needs some more investigation... > > By printing the generated char array to stdout with a small C program I can > see that in the case of python3 the JS is not minified. The issue is caused because all over the function JavascriptMinify:minify() the code does comparisons with strings, which fail on python3 because now the type for the variable is bytes b'/' == '/' is False Something like this is needed: --- a/Source/JavaScriptCore/Scripts/jsmin.py +++ b/Source/JavaScriptCore/Scripts/jsmin.py @@ -82,11 +82,17 @@ class JavascriptMinify(object): if str(char) in 'return': self.return_buf += char self.is_return = self.return_buf == 'return' + if is_3: + char=str.encode(char) self.outs.write(char) if self.is_return: self.return_buf = '' - read = self.ins.read + def read(char): + if is_3: + return self.ins.read(char).decode() + else: + return self.ins.read(char) Maybe there is a better way of doing this that doesn't require converting between str and bytes back and fort?
Ah I missed the read part earlier. I think the simplest is likely to convert byte strings to utf-8 in Python3, so that the Python2 code can work as-is?
I may have an updated patch but it needs some more testing wrt l10n files.
Created attachment 376851 [details] WiP patch This is my current status. However I would like to move this jsmin thing to webkitpy and add unit-tests. But right now I don't have time for this, so unless someone else picks up the task I will try to get back to it soon.
(In reply to Philippe Normand from comment #50) > Created attachment 376851 [details] > WiP patch > > This is my current status. However I would like to move this jsmin thing to > webkitpy and add unit-tests. But right now I don't have time for this, so > unless someone else picks up the task I will try to get back to it soon. I don't think moving this to webkitpy is a good idea. This needs to be shipped on release tarballs as is part of the built, and release tarballs don't include webkitpy (AFAIK). On the other hand the unit tests for webkitpy are currently ran only with python2, we don't have any facility to run unit tests with python3. Maybe an idea is to test this from webkitpy (without moving it, simply call it where is it from webkitpy) and also start testing webkitpy with python3 (on top of python2). But this is not going to be easy and is already out of the scope of this bug. I suggest to simply fix the issue here and after that (if you want), on a new bug, start working on this extra tests with python3.
Comment on attachment 376851 [details] WiP patch View in context: https://bugs.webkit.org/attachment.cgi?id=376851&action=review Overall patch idea LGTM, but I think there is one situation which is not being covered — please check the comment. I agree with Carlos López that it would be good to try things with Python 3.x, but that is indeed scope for a different bug. > Source/JavaScriptCore/Scripts/jsmin.py:56 > + ins = klass(js.readall()) This is problematic, because from the code above which checks whether “js” is an “unicode” instance, it looks to me that it is acceptable to pass a string to “jsmin()” — I think something like the following would be better: text_class = str if is_3 else unicode bytes_class = bytes if is_3 else str if hasattr(js, "readall"): js = js.readall() elif isinstance(js, text_class): js = js.encode("utf-8") # In the end we always want a sequence of bytes. assert isinstance(js, bytes_class) ins = klass(js) # ... I think something in the spirit of the example above should cover all bases.
FWIW, shouldn't we tag this with [WPE] as well? I was under the impression that WPE uses media controls with CSS/JS.
Actually I suppose that the Apple ports are also affected by this issue.
(In reply to Philippe Normand from comment #54) > Actually I suppose that the Apple ports are also affected by this issue. Yes, that will be the case when they start using Python 3.x for building later on; IIRC for the moment they are still using 2.x
I think I have a patch that fixes this :) I tested to run manually the command make-js-file-arrays.py with different source files and compared the outputs for python2 and python3. With this patch it is generating always the same result as far as I was able to test. I also tested to match with the output it was generating before this patch for python2 and it matches. I also compared the generated sources after a build before and after this patch, and with python2 and python3, and the sources affected by this seem to match exactly as expected. And of course this bug and bug 200894 are fixed with this. I'll upload it now for review/test.
Created attachment 377080 [details] Patch
Comment on attachment 377080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377080&action=review > Source/JavaScriptCore/Scripts/jsmin.py:94 > + raise ValueError("ERROR: The script jsmin.py only can handle text input, but it received input with type %s" % type(char)) I think it should be "can only handle" and "received input of type"
Committed r249095: <https://trac.webkit.org/changeset/249095>
<rdar://problem/54706396>