Rafostar / gtuber

C library to fetch media info from websites
https://rafostar.github.io/gtuber/
GNU Lesser General Public License v2.1
9 stars 3 forks source link

List of GStreamer integration issues #36

Open Rafostar opened 1 year ago

Rafostar commented 1 year ago

Gtuber additionally includes a GStreamer plugin for integration with GStreamer-based applications. Unfortunately due to few bugs in GStreamer it is not working as it should. Here is a list of issues preventing it from working:

dashdemux:

  1. mpdparser loses URI query part - Causes some services to not work at all. Fix here.
  2. segmentbase type with sidx is not working as expected - Causes connection throttling. Issue here, fix was submitted as a patch in said issue. Rebased version can be found here.
  3. streaming starts always from worst quality - Fix can be found here.

adaptivedemux2:

  1. not all request headers are set - Currently only User-Agent, Referer and Cookie request headers fields are set. Any other are lost. Attempt at fixing/discussion here. Currently not considered a major problem here, since the most important ones that we need (user-agent, cookies, referer) are supported.

dashdemux2:

  1. mpdparser loses URI query part - Same problem as in original dashdemux, since this code was copied from it.
  2. streaming starts always from worst quality - Same problem as in original dashdemux, since this code was copied from it. Also start-bitrate property is missing to make this even easier to control. Fix can be found here.
  3. last subsegment doesn't play - When watching video, last few seconds are missing.

hlsdemux2:

  1. start-bitrate property does not work without setting connection-speed - It would be simpler and more efficient if we could just set start-bitrate prop and continue with adaptive streaming. Fix can be found here.

NOTES:

josch commented 1 year ago

I talked with Nicolas Dufresne on IRC (he is known as ndufresne on irc.libera.chat) about this and he said that dash (and the others) got reimplemented by bilboed recently. Maybe reach out to ndufresne on IRC. He is usually very responsive and easy to approach. :)

Rafostar commented 1 year ago

I already mentioned dashdemux2 existence in notes above. In fact I asked about the functionality I have been using in original dashdemux on their IRC some time ago. Back then Tim from GStreamer said "I think it will require something added".

One thing that concerns me is that new implementation works only with playbin3, so it would be plausible for me to get old one working anyway. I am always open to discussion. Currently the possibilities I see are:

  1. Find someone to review and merge those 3 patches upstream from GStreamer team.
  2. Test dashdemux2, add missing functionality and find someone to convince this is needed, review and merge it upstream.
  3. Ship a patched and renamed copy of dashdemux element as part of this project.
  4. Ship a patched and renamed copy of dashdemux2 element as part of this project.

I would rather prefer to avoid 3, as all I need there are bug fixes and not adding new functionalities (and I really do not want to maintain it in sync with upstream here). Similarly for option 4, but I can consider it if they will disagree to add the functionality I need. Both 1 and 2 depend on finding a maintainer that can merge upstream unfortunately and 2 requires additional work from me that I haven't had time to look into yet (wasn't high priority as its only in gstreamer git right now).

Rafostar commented 1 year ago

Actually, taking closer look at dashdemux2 source code, it seems that the functionality I want might already be there. Gonna have to find more free time to add support for it here and test it through.

Update: It is there, but seems incomplete. We need an ability to set whatever HTTP request headers we need (like we can in other HTTP source elements in GStreamer and old dashdemux which uses those). New implementation only allows few for some unknown reason.

josch commented 1 year ago

This patch is incomplete. I tried applying it to gst-plugins-bad1.0 in Debian but the patch changes the function signature of gst_mpd_client_setup_streaming without fixing all calls to the function throughout the codebase, so compilation aborts with errors like:

FAILED: tests/check/elements_dash_mpd.p/elements_dash_mpd.c.o 
cc -Itests/check/elements_dash_mpd.p -Itests/check -I../tests/check -I. -I.. -Igst-libs -I../gst-libs -Igst-libs/gst/basecamerabinsrc -Igst-libs/gst/interfaces -I/usr/include/gstreamer-1.0 -I/usr/include/aarch64-linux-gnu -I/usr/include/glib-2.0 -I/usr/lib/aarch64-linux-gnu/glib-2.0/include -I/usr/include/orc-0.4 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/libxml2 -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O0 -fvisibility=hidden -fno-strict-aliasing -DG_DISABLE_CAST_CHECKS -Wmissing-prototypes -Wdeclaration-after-statement -Wold-style-definition -Wmissing-declarations -Wredundant-decls -Wwrite-strings -Wformat -Wformat-security -Winit-self -Wmissing-include-dirs -Waddress -Wno-multichar -Wvla -Wpointer-arith -g -O2 -ffile-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -Wno-error -Wdate-time -D_FORTIFY_SOURCE=2 -pthread -DHAVE_CONFIG_H -UG_DISABLE_ASSERT -UG_DISABLE_CAST_CHECKS '-DGST_CHECK_TEST_ENVIRONMENT_BEACON="GST_STATE_IGNORE_ELEMENTS"' '-DGST_TEST_FILES_PATH="/<<PKGBUILDDIR>>/tests/check/../files"' '-DTEST_PATH="/<<PKGBUILDDIR>>/obj-aarch64-linux-gnu/tests/check/media"' -DDASH_MPD_DATADIR=/<<PKGBUILDDIR>>/tests/check/elements/dash_mpd_data -DGST_USE_UNSTABLE_API -MD -MQ tests/check/elements_dash_mpd.p/elements_dash_mpd.c.o -MF tests/check/elements_dash_mpd.p/elements_dash_mpd.c.o.d -o tests/check/elements_dash_mpd.p/elements_dash_mpd.c.o -c ../tests/check/elements/dash_mpd.c
../tests/check/elements/dash_mpd.c: In function ‘dash_mpdparser_period_segmentTemplateWithPresentationTimeOffset’:
../tests/check/elements/dash_mpd.c:934:9: error: too few arguments to function ‘gst_mpd_client_setup_streaming’
  934 |   ret = gst_mpd_client_setup_streaming (mpdclient, adapt_set);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../tests/check/elements/dash_mpd.c:49:
../tests/check/../../ext/dash/gstmpdclient.c:1599:1: note: declared here

Given your evaluation of dashdemux2, do you think I should invest time into fixing the patch so that it builds successfully?

Rafostar commented 1 year ago

The patch itself is working and compiles fine since I ship it in Flatpak, but since it wasn't send, it seems I forgot to update unit tests accordingly (that aren't built there). This one is actually optional. It improves what mentioned in this issue list, but is not required (unlike other two).

Given your evaluation of dashdemux2, do you think I should invest time into fixing the patch so that it builds successfully?

Depends on you. I did some limited testing recently with little bit of their code browsing, and adaptivedemux2 class seems like a very good improvement over the old one. Since it basically deprecated old implementation, it seems plausible to me to work towards shifting to the new one here as long as it has all the needed functionality/works as expected. But this will be a future GStreamer 1.22 material.

I actually have 2 elements shipped here that are just adding missing functionality to old implementation and because they use it internally, they do not work correctly anyway due to those bugs. If GStreamer will have newer and better implementation that doesn't need me to add more to it, I would gladly drop them (since I can now before 1.0 tag) in favor of using stock upstream versions.

Since adding support for the new ones doesn't involve that much work, I hope I will be able to finish it on the weekend. I will keep updating this issue, adding any problems I find in new implementation to the list.

Rafostar commented 1 year ago

@josch

In case of dashdemux2, I made this MR and there is ongoing discussion how to handle this best in it. This is about my proposition about adding missing stuff to that element. Only lack of custom request headers seems missing currently.

ATM dashdemux2 element is still new and seems to have more different weird bugs then old one. And I cannot conveniently add missing functionality to it like I did for the old one here. It will probably also need some other fixes along the way, but adding what's needed comes first.

As a last resort (I really do not want to maintain this unless necessary), option number 3 is still valid.

Rafostar commented 1 year ago

Strikethrough lines are the fixes that are needed, but were already merged upstream. I managed to get 1 fix in recently :smile:.

@josch I dunno if you are still interested in this project or patches, but for the info:

josch commented 1 year ago

Thank you for pinging me @Rafostar! Somehow, even though I subscribed to this issue, I do not get notified once you edit the text. I'm definitely still interested in clapper and gtuber because all ffmpeg based media players do not support v4l2 hardware decoding on my platform (imx8mq with hantro). So this is the only way I can properly watch youtube and twitch. Thank you for maintaining it! I'll check out the updated patches again and apply them on top of the gstreamer Debian packages.

Rafostar commented 1 year ago

Latest git now supports and automatically uses new dashdemux2/hlsdemux2 elements from GStreamer 1.22 release when playbin3 is used.

josch commented 1 year ago

I was just re-reading this issue as well right now! :)

gstreamer 1.22 just got into Debian as well and now I can drop some of your patches that were included in the 1.22 release. Nice!!

Rafostar commented 1 year ago

@josch

Thanks for still being here and maintaining this!

gstreamer 1.22 just got into Debian as well and now I can drop some of your patches that were included in the 1.22 release. Nice!!

In this case, as remaining two dashdemux2 problems left (low start quality and missing last segment). For the first (low start quality) I will rebase and update my MR, but can't today. For the later one there seems to be some off by 1 mistake going on in GStreamer, for example if I do something like:

diff --git a/subprojects/gst-plugins-good/ext/adaptivedemux2/dash/gstdashdemux.c b/subprojects/gst-plugins-good/ext/adaptivedemux2/dash/gstdashdemux.c
index ab7445f532..b2a8a801e9 100644
--- a/subprojects/gst-plugins-good/ext/adaptivedemux2/dash/gstdashdemux.c
+++ b/subprojects/gst-plugins-good/ext/adaptivedemux2/dash/gstdashdemux.c
@@ -1634,7 +1634,7 @@ gst_dash_demux_stream_has_next_subfragment (GstAdaptiveDemux2Stream * stream)
   if (dashstream->sidx_parser.status == GST_ISOFF_SIDX_PARSER_FINISHED) {
     gboolean playing_forward = (stream->demux->segment.rate > 0.0);
     if (playing_forward) {
-      if (sidx->entry_index + 1 < sidx->entries_count)
+      if (sidx->entry_index < sidx->entries_count)
         return TRUE;
     } else {
       if (sidx->entry_index >= 1)

Then whole video will play without issue. I will need to open them an issue about it, but would be nice to find and reproduce with some public DASH manifest sample first. Maybe one of these will do: https://qiita.com/yun_bow/items/780e49531174272e7c44

Rafostar commented 1 year ago

Got hlsdemux2 "start-bitrate" MR merged today. Hopefully I will menage to also upstream similar change for dashdemux2 in near future, then we will be able to benefit from these changes here.

Rafostar commented 1 year ago

Got dashdemux2 startup video quality improvements merged today too 😄.