Bug 171595

Summary: [GTK] Bump GStreamer version to 1.10.4 in jhbuild
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, clopez, commit-queue, eocanha, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 171681    
Bug Blocks: 170500    
Attachments:
Description Flags
Patch
calvaris: review-
Updated patch none

Description Carlos Garcia Campos 2017-05-03 03:25:04 PDT
Upgrade from 1.8 to 1.10
Comment 1 Carlos Garcia Campos 2017-05-03 03:34:10 PDT
I've tried to upgrade, removing all patches (I don't know if any of those would still be needed with 1.10, though). And the results are not that bad, there some unexpected failures, but also unexpected passes. So, I think we can land the upgrade and deal with the new failures.

The relevant media results are

Regressions: Unexpected text-only failures
  imported/w3c/web-platform-tests/media-source/mediasource-redundant-seek.html [ Failure ]
  imported/w3c/web-platform-tests/media-source/mediasource-removesourcebuffer.html [ Failure ]
  imported/w3c/web-platform-tests/media-source/mediasource-seek-beyond-duration.html [ Failure ]
  imported/w3c/web-platform-tests/media-source/mediasource-seek-during-pending-seek.html [ Failure ]
  media/media-source/media-source-init-segment-duration.html [ Failure ]
  media/video-error-does-not-exist.html [ Failure ]
  media/video-load-networkState.html [ Failure ]
  media/video-source-error.html [ Failure ]
  media/video-source-none-supported.html [ Failure ]

Regressions: Unexpected audio failures
  webaudio/codec-tests/wav/24bit-22khz-resample.html [ Failure ]

Regressions: Unexpected crashes
  http/tests/media/video-accept-encoding.html [ Crash ]

Regressions: Unexpected timeouts
  media/video-source-moved.html [ Timeout ]

Unexpected flakiness: timeouts
  media/W3C/audio/events/event_canplaythrough.html [ Timeout Pass ]
  media/W3C/video/events/event_loadedmetadata_manual.html [ Timeout Pass ]
  media/W3C/video/events/event_timeupdate_manual.html [ Timeout Pass ]
  media/W3C/video/readyState/readyState_during_playing.html [ Timeout Pass ]
  media/track/track-in-band-legacy-api.html [ Failure Timeout ]
Comment 2 Carlos Garcia Campos 2017-05-03 03:41:34 PDT
Created attachment 308892 [details]
Patch
Comment 3 Enrique Ocaña 2017-05-03 04:04:15 PDT
At least these two patches might still be important. We're keeping them in out downstream port, which also uses GStreamer  1.10.4:

* gtk/patches/gst-plugins-good-0004-qtdemux-add-context-for-a-preferred-protection.patch
* gtk/patches/gstreamer-0001-protection-added-function-to-filter-system-ids.patch
Comment 4 Carlos Garcia Campos 2017-05-03 04:14:04 PDT
(In reply to Enrique Ocaña from comment #3)
> At least these two patches might still be important. We're keeping them in
> out downstream port, which also uses GStreamer  1.10.4:
> 
> *
> gtk/patches/gst-plugins-good-0004-qtdemux-add-context-for-a-preferred-
> protection.patch
> *
> gtk/patches/gstreamer-0001-protection-added-function-to-filter-system-ids.
> patch

Thanks Quique! Do you know which tests those patches fix?
Comment 5 Xabier Rodríguez Calvar 2017-05-03 04:28:14 PDT
(In reply to Carlos Garcia Campos from comment #4)
> > *
> > gtk/patches/gst-plugins-good-0004-qtdemux-add-context-for-a-preferred-
> > protection.patch
> > *
> > gtk/patches/gstreamer-0001-protection-added-function-to-filter-system-ids.
> > patch
> 
> Thanks Quique! Do you know which tests those patches fix?

None yet, it's EME code.
Comment 6 Xabier Rodríguez Calvar 2017-05-03 04:42:58 PDT
Comment on attachment 308892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308892&action=review

> Tools/ChangeLog:17
> +        * gtk/patches/gst-plugins-bad-0001-dtls-port-to-OpenSSL-1.1.0.patch: Removed.
> +        * gtk/patches/gst-plugins-bad-0002-dtlscertificate-Fix-error-checking-in-RSA_generate_k.patch: Removed.
> +        * gtk/patches/gst-plugins-good-0001-rtpbin-pipeline-gets-an-EOS-when-any-rtpsources-byes.patch: Removed.
> +        * gtk/patches/gst-plugins-good-0002-rtpbin-avoid-generating-errors-when-rtcp-messages-ar.patch: Removed.
> +        * gtk/patches/gst-plugins-good-0003-rtpbin-receive-bundle-support.patch: Removed.
> +        * gtk/patches/gst-plugins-good-0004-qtdemux-add-context-for-a-preferred-protection.patch: Removed.
> +        * gtk/patches/gst-plugins-good-Revert-qtdemux-expose-streams-with-first-moof-for-fr.patch: Removed.
> +        * gtk/patches/gst-plugins-good-use-the-tfdt-decode-time.patch: Removed.
> +        * gtk/patches/gstreamer-0001-protection-added-function-to-filter-system-ids.patch: Removed.

Please, check if these patches are present in 1.10.4 before removing them. As I said at least two of them are still needed. Please keep me posted.

> Tools/gtk/jhbuild.modules:-361
> -  <autotools id="orc" autogenargs="--disable-gtk-doc" autogen-sh="configure">
> -    <branch module="orc/orc-0.4.17.tar.gz" version="0.4.17"
> -            repo="gstreamer"
> -            hash="sha256:4fc7cca48c59fff23afee78fb642cdbde001f56401c8f47b95a16578d1d5d7e8"
> -            md5sum="af1bf3dab9e69f3c36f389285e2a12a1"/>
> -  </autotools>

Why are you removing orc?
Comment 7 Xabier Rodríguez Calvar 2017-05-03 04:43:41 PDT
(In reply to Xabier Rodríguez Calvar from comment #6)
> Please, check if these patches are present in 1.10.4 before removing them.
> As I said at least two of them are still needed. Please keep me posted.

And a test status update would be interesting too.
Comment 8 Carlos Garcia Campos 2017-05-03 05:01:24 PDT
(In reply to Xabier Rodríguez Calvar from comment #5)
> (In reply to Carlos Garcia Campos from comment #4)
> > > *
> > > gtk/patches/gst-plugins-good-0004-qtdemux-add-context-for-a-preferred-
> > > protection.patch
> > > *
> > > gtk/patches/gstreamer-0001-protection-added-function-to-filter-system-ids.
> > > patch
> > 
> > Thanks Quique! Do you know which tests those patches fix?
> 
> None yet, it's EME code.

We don't maintain patches to fix tests that don't exist.
Comment 9 Carlos Garcia Campos 2017-05-03 05:03:05 PDT
(In reply to Xabier Rodríguez Calvar from comment #6)
> Comment on attachment 308892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308892&action=review
> 
> > Tools/ChangeLog:17
> > +        * gtk/patches/gst-plugins-bad-0001-dtls-port-to-OpenSSL-1.1.0.patch: Removed.
> > +        * gtk/patches/gst-plugins-bad-0002-dtlscertificate-Fix-error-checking-in-RSA_generate_k.patch: Removed.
> > +        * gtk/patches/gst-plugins-good-0001-rtpbin-pipeline-gets-an-EOS-when-any-rtpsources-byes.patch: Removed.
> > +        * gtk/patches/gst-plugins-good-0002-rtpbin-avoid-generating-errors-when-rtcp-messages-ar.patch: Removed.
> > +        * gtk/patches/gst-plugins-good-0003-rtpbin-receive-bundle-support.patch: Removed.
> > +        * gtk/patches/gst-plugins-good-0004-qtdemux-add-context-for-a-preferred-protection.patch: Removed.
> > +        * gtk/patches/gst-plugins-good-Revert-qtdemux-expose-streams-with-first-moof-for-fr.patch: Removed.
> > +        * gtk/patches/gst-plugins-good-use-the-tfdt-decode-time.patch: Removed.
> > +        * gtk/patches/gstreamer-0001-protection-added-function-to-filter-system-ids.patch: Removed.
> 
> Please, check if these patches are present in 1.10.4 before removing them.
> As I said at least two of them are still needed. Please keep me posted.

I don't understand why those two tests are needed if they don't fix any exiting test.

> > Tools/gtk/jhbuild.modules:-361
> > -  <autotools id="orc" autogenargs="--disable-gtk-doc" autogen-sh="configure">
> > -    <branch module="orc/orc-0.4.17.tar.gz" version="0.4.17"
> > -            repo="gstreamer"
> > -            hash="sha256:4fc7cca48c59fff23afee78fb642cdbde001f56401c8f47b95a16578d1d5d7e8"
> > -            md5sum="af1bf3dab9e69f3c36f389285e2a12a1"/>
> > -  </autotools>
> 
> Why are you removing orc?

Because the version in recent distros is even higher than that one. 0.4.22 in debian, for example.
Comment 10 Carlos Alberto Lopez Perez 2017-05-03 05:05:29 PDT
Comment on attachment 308892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308892&action=review

> Tools/ChangeLog:13
> +        * gtk/patches/gst-plugins-good-0003-rtpbin-receive-bundle-support.patch: Removed.

This one is not merged and is needed Webrtc. Please recover it.
Comment 11 Xabier Rodríguez Calvar 2017-05-03 08:32:09 PDT
(In reply to Carlos Garcia Campos from comment #8)
> We don't maintain patches to fix tests that don't exist.

The feature is under development and we can't work without it so please, leave those two there.

(In reply to Carlos Garcia Campos from comment #9)
> I don't understand why those two tests are needed if they don't fix any
> exiting test.

As I said, feature is under heavy development and we need them.

> > > Tools/gtk/jhbuild.modules:-361
> > > -  <autotools id="orc" autogenargs="--disable-gtk-doc" autogen-sh="configure">
> > > -    <branch module="orc/orc-0.4.17.tar.gz" version="0.4.17"
> > > -            repo="gstreamer"
> > > -            hash="sha256:4fc7cca48c59fff23afee78fb642cdbde001f56401c8f47b95a16578d1d5d7e8"
> > > -            md5sum="af1bf3dab9e69f3c36f389285e2a12a1"/>
> > > -  </autotools>
> > 
> > Why are you removing orc?
> 
> Because the version in recent distros is even higher than that one. 0.4.22
> in debian, for example.

Ok.

(In reply to Carlos Alberto Lopez Perez from comment #10)
> This one is not merged and is needed Webrtc. Please recover it.

Yes, basically all patches that are not in GStreamer yet are needed, either for tests or for features that are being developed. Please, check the availability of those patches in the required version, rerun the tests and resubmit the patch.
Comment 12 Carlos Garcia Campos 2017-05-03 08:37:01 PDT
(In reply to Xabier Rodríguez Calvar from comment #11)
> (In reply to Carlos Garcia Campos from comment #8)
> > We don't maintain patches to fix tests that don't exist.
> 
> The feature is under development and we can't work without it so please,
> leave those two there.
> 
> (In reply to Carlos Garcia Campos from comment #9)
> > I don't understand why those two tests are needed if they don't fix any
> > exiting test.
> 
> As I said, feature is under heavy development and we need them.
> 
> > > > Tools/gtk/jhbuild.modules:-361
> > > > -  <autotools id="orc" autogenargs="--disable-gtk-doc" autogen-sh="configure">
> > > > -    <branch module="orc/orc-0.4.17.tar.gz" version="0.4.17"
> > > > -            repo="gstreamer"
> > > > -            hash="sha256:4fc7cca48c59fff23afee78fb642cdbde001f56401c8f47b95a16578d1d5d7e8"
> > > > -            md5sum="af1bf3dab9e69f3c36f389285e2a12a1"/>
> > > > -  </autotools>
> > > 
> > > Why are you removing orc?
> > 
> > Because the version in recent distros is even higher than that one. 0.4.22
> > in debian, for example.
> 
> Ok.
> 
> (In reply to Carlos Alberto Lopez Perez from comment #10)
> > This one is not merged and is needed Webrtc. Please recover it.
> 
> Yes, basically all patches that are not in GStreamer yet are needed, either
> for tests or for features that are being developed. Please, check the
> availability of those patches in the required version, rerun the tests and
> resubmit the patch.

I wish I had the time, I don't know when I'll find some more time to do all that
Comment 13 Carlos Alberto Lopez Perez 2017-05-03 11:59:32 PDT
Comment on attachment 308892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308892&action=review

I have checked the patches, I comment below inline which ones should be kept or removed

> Tools/gtk/jhbuild.modules:-374
> -      <patch file="gstreamer-0001-protection-added-function-to-filter-system-ids.patch" strip="1"/>

I don't see it merged anywhere neither any bug related to it. I guess its still a WIP.
Keep this patch.

> Tools/gtk/jhbuild.modules:-406
> -      <patch file="gst-plugins-good-use-the-tfdt-decode-time.patch" strip="1"/>

Already merged in 1.10.4 : https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=a2adb7e30
Remove it

> Tools/gtk/jhbuild.modules:-407
> -      <patch file="gst-plugins-good-Revert-qtdemux-expose-streams-with-first-moof-for-fr.patch" strip="1"/>

Already merged in 1.10.4 : https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=707c69cb7
Remove it

> Tools/gtk/jhbuild.modules:-408
> -      <patch file="gst-plugins-good-0001-rtpbin-pipeline-gets-an-EOS-when-any-rtpsources-byes.patch" strip="1"/>

Merged in 1.10.4 : https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=7ec1ba2bf
Note: This was reverted on master, but not on 1.10.4 .. this means we may need to recover it on the future if we update beyond 1.10.4 and the patch doesn't gets merged again upstream.
For the moment (update to 1.10.4) we should just simply remove it as it is merged on 1.10.4
Remove it

> Tools/gtk/jhbuild.modules:-409
> -      <patch file="gst-plugins-good-0002-rtpbin-avoid-generating-errors-when-rtcp-messages-ar.patch" strip="1"/>

Already merged in 1.10.4 : https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=7708a1793
Remove it

> Tools/gtk/jhbuild.modules:-410
> -      <patch file="gst-plugins-good-0003-rtpbin-receive-bundle-support.patch" strip="1"/>

This was merged on https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=dcd3ce9751cdef0b5ab1fa118355f92bdfe82cb3 (>= 1.11.1)
So, we need to keep this patch because its not on 1.10.4
Keep this patch.

> Tools/gtk/jhbuild.modules:-411
> -      <patch file="gst-plugins-good-0004-qtdemux-add-context-for-a-preferred-protection.patch" strip="1"/>

I don't see it merged anywhere neither any bug related to it. I guess its still a WIP.
Keep this patch.

> Tools/gtk/jhbuild.modules:-427
> -      <patch file="gst-plugins-bad-0001-dtls-port-to-OpenSSL-1.1.0.patch" strip="1"/>

Already merged in 1.10.4 : https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=e93893316
Remove it

> Tools/gtk/jhbuild.modules:-428
> -      <patch file="gst-plugins-bad-0002-dtlscertificate-Fix-error-checking-in-RSA_generate_k.patch" strip="1"/>

Already merged in 1.10.4 : https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=b810d09fd
Remove it
Comment 14 Michael Catanzaro 2017-05-03 13:42:55 PDT
So we have two patches without any upstream bug report?
Comment 15 Carlos Alberto Lopez Perez 2017-05-03 13:50:15 PDT
(In reply to Michael Catanzaro from comment #14)
> So we have two patches without any upstream bug report?

According to the previous conversation those patches are EME related, and the feature is still under development. Most likely the patches are still not ready to be proposed to upstream.

But I'm just guessing here, if you want more details ask to the authors of the patches.
Comment 16 Carlos Garcia Campos 2017-05-03 23:09:01 PDT
(In reply to Carlos Alberto Lopez Perez from comment #13)
> Comment on attachment 308892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308892&action=review
> 
> I have checked the patches, I comment below inline which ones should be kept
> or removed

Wow, thanks for doing the boring work!

> > Tools/gtk/jhbuild.modules:-374
> > -      <patch file="gstreamer-0001-protection-added-function-to-filter-system-ids.patch" strip="1"/>
> 
> I don't see it merged anywhere neither any bug related to it. I guess its
> still a WIP.
> Keep this patch.

I still don't understand why we keep patches for our dependencies that are not tracked upstream and don't fix any test.

> > Tools/gtk/jhbuild.modules:-406
> > -      <patch file="gst-plugins-good-use-the-tfdt-decode-time.patch" strip="1"/>
> 
> Already merged in 1.10.4 :
> https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=a2adb7e30
> Remove it
> 
> > Tools/gtk/jhbuild.modules:-407
> > -      <patch file="gst-plugins-good-Revert-qtdemux-expose-streams-with-first-moof-for-fr.patch" strip="1"/>
> 
> Already merged in 1.10.4 :
> https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=707c69cb7
> Remove it
> 
> > Tools/gtk/jhbuild.modules:-408
> > -      <patch file="gst-plugins-good-0001-rtpbin-pipeline-gets-an-EOS-when-any-rtpsources-byes.patch" strip="1"/>
> 
> Merged in 1.10.4 :
> https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=7ec1ba2bf
> Note: This was reverted on master, but not on 1.10.4 .. this means we may
> need to recover it on the future if we update beyond 1.10.4 and the patch
> doesn't gets merged again upstream.
> For the moment (update to 1.10.4) we should just simply remove it as it is
> merged on 1.10.4
> Remove it
> 
> > Tools/gtk/jhbuild.modules:-409
> > -      <patch file="gst-plugins-good-0002-rtpbin-avoid-generating-errors-when-rtcp-messages-ar.patch" strip="1"/>
> 
> Already merged in 1.10.4 :
> https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=7708a1793
> Remove it
> 
> > Tools/gtk/jhbuild.modules:-410
> > -      <patch file="gst-plugins-good-0003-rtpbin-receive-bundle-support.patch" strip="1"/>
> 
> This was merged on
> https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/
> ?id=dcd3ce9751cdef0b5ab1fa118355f92bdfe82cb3 (>= 1.11.1)
> So, we need to keep this patch because its not on 1.10.4
> Keep this patch.
> 
> > Tools/gtk/jhbuild.modules:-411
> > -      <patch file="gst-plugins-good-0004-qtdemux-add-context-for-a-preferred-protection.patch" strip="1"/>
> 
> I don't see it merged anywhere neither any bug related to it. I guess its
> still a WIP.
> Keep this patch.
> 
> > Tools/gtk/jhbuild.modules:-427
> > -      <patch file="gst-plugins-bad-0001-dtls-port-to-OpenSSL-1.1.0.patch" strip="1"/>
> 
> Already merged in 1.10.4 :
> https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=e93893316
> Remove it
> 
> > Tools/gtk/jhbuild.modules:-428
> > -      <patch file="gst-plugins-bad-0002-dtlscertificate-Fix-error-checking-in-RSA_generate_k.patch" strip="1"/>
> 
> Already merged in 1.10.4 :
> https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=b810d09fd
> Remove it
Comment 17 Xabier Rodríguez Calvar 2017-05-04 00:28:06 PDT
(In reply to Michael Catanzaro from comment #14)
> So we have two patches without any upstream bug report?

There's a bug report in GStreamer.
Comment 18 Carlos Garcia Campos 2017-05-04 01:31:26 PDT
Created attachment 309026 [details]
Updated patch

Results are the same as with the previous patch.
Comment 19 Xabier Rodríguez Calvar 2017-05-04 04:07:27 PDT
Comment on attachment 309026 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309026&action=review

> Tools/ChangeLog:13
> +        * gtk/patches/gst-plugins-good-0004-qtdemux-add-context-for-a-preferred-protection.patch:

Nit: If you want to mark it as rebased, perfect.
Comment 20 WebKit Commit Bot 2017-05-04 04:57:11 PDT
Comment on attachment 309026 [details]
Updated patch

Clearing flags on attachment: 309026

Committed r216179: <http://trac.webkit.org/changeset/216179>
Comment 21 WebKit Commit Bot 2017-05-04 04:57:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Michael Catanzaro 2017-05-08 21:02:11 PDT
I updated the expectation for webaudio/codec-tests/wav/24bit-22khz-resample-expected.wav in http://trac.webkit.org/changeset/216476/webkit, because I can't hear any difference. Please yell if that was wrong.