After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 763278 - tcpserversink: problems with TS muxed stream, not detecting/sending keyframes
tcpserversink: problems with TS muxed stream, not detecting/sending keyframes
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.7.90
Other Linux
: Normal blocker
: 1.7.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-03-08 01:16 UTC by Peter Maersk-Moller
Modified: 2016-03-14 11:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
multihandlesink: Forward HEADER buffers if the caps don't contain a streamheader field (7.23 KB, patch)
2016-03-12 10:49 UTC, Sebastian Dröge (slomo)
needs-work Details | Review
multihandlesink: Only don't send HEADER buffers normally if they are actually streamheaders from the caps (4.69 KB, patch)
2016-03-13 09:00 UTC, Sebastian Dröge (slomo)
none Details | Review
tcp: Remove unused file (94.92 KB, patch)
2016-03-13 09:01 UTC, Sebastian Dröge (slomo)
committed Details | Review
multihandlesink: Only don't send HEADER buffers normally if they are actually streamheaders from the caps (4.79 KB, patch)
2016-03-13 09:10 UTC, Sebastian Dröge (slomo)
none Details | Review
multihandlesink: Only don't send HEADER buffers normally if they are actually streamheaders from the caps (5.42 KB, patch)
2016-03-13 09:19 UTC, Sebastian Dröge (slomo)
none Details | Review
multihandlesink: Only don't send HEADER buffers normally if they are actually streamheaders from the caps (4.95 KB, patch)
2016-03-13 11:57 UTC, Sebastian Dröge (slomo)
committed Details | Review
multihandlesink: Remove useless streamheader storage (5.12 KB, patch)
2016-03-13 12:02 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Peter Maersk-Moller 2016-03-08 01:16:54 UTC
I have a problem with the 1.7.90 version for the pipeline shown below. The
pipeline works fine for 1.6.3, but for 1.7.90, nothing comes out of the
tcpserversink although plenty of packets flows into it.

The flow out of tcpserversink is initally tested with the command

        nc localhost 5010 | od -c

and subesequently tested with both vlc and a gstreamer pipeline with
tcpclientsrc.

If the tcpserversink option 'sync-method=2' is removed from the pipeline,
then version 1.7.90 tcpserversink will send data, but VLC can never decode
it successfully. If the tcpserversink option 'sync-method=2' is removed
from the pipeline for 1.6.3, VLC and others can still play the stream, but
it can take longer to find the first image to decode.

Similar issues have been seen before where the problem usually has been
mpegtsmux not setting flags correct or in a way tcpserversink would expect.

Note that the live pipeline sets timestamps upon input and subseqently the
timestamps do not start from zero.

Think this needs to be resolved before 1.8.0 is released so we don't get a
stable release out there with a tsmux/tcpserversink issue, but that is just
a suggestion.

Below is the pipeline. I assume you would be able to create pipelines with audiotestsrc and videotestsrc and feed these into the pipeline below with fdsink and shmsink to emulate data into the pipeline NOT starting at time=0.


/usr/local/bin/gst-launch-1.0 -v fdsrc fd=0 do-timestamp=true ! queue !\
        audio/x-raw,format=S16LE,layout=interleaved, rate=44100,channels=2
!\
        queue ! audioconvert ! audiorate !\
        faac bitrate=128000 !\
        audio/mpeg,mpegversion=4, stream-format=raw !\
        aacparse ! queue !\
        muxer. shmsrc socket-path=/tmp/mixer1 do-timestamp=true
is-live=true !\

video/x-raw,format=BGRA,pixel-aspect-ratio=1/1,interlace-mode=progressive,width=1280,height=720,framerate=30/1
!\
        queue max-size-bytes=0 leaky=2 max-size-buffers=30 !\
        videoconvert ! queue !\
        x264enc bitrate=3499 tune=zerolatency speed-preset=6 key-int-max=60
bframes=0 !\
        video/x-h264,alignment=au,stream-format=byte-stream,profile=main !\
        h264parse ! queue !\
        mpegtsmux name=muxer !\
        tsparse ! queue ! tee name=tstream !\
        rtpmp2tpay !\
        udpsink host=127.0.0.1 port=10072 sync=false tstream. !\
        tcpserversink port=5010 host=0.0.0.0 sync-method=2

See also https://lists.freedesktop.org/archives/gstreamer-devel/2016-March/057071.html

The bug is filed under tcpserversink, but I would guess it is an issue with mpegtsmux/tcpserversink signalling, where tcpserversink may not recognize correctly the flags mpegtsmux sets for identifying I-frames or it may be something with timing.

The bug is marked for Linux, but that is because I have only tested for the error on Linux.

Best regards
Peter Maersk-Moller
Comment 1 Sebastian Dröge (slomo) 2016-03-08 07:33:48 UTC
Can you "git bisect" to find the commit(s) that cause this?
Comment 2 Peter Maersk-Moller 2016-03-08 10:39:31 UTC
(In reply to Sebastian Dröge (slomo) from comment #1)
> Can you "git bisect" to find the commit(s) that cause this?

Probably, if you tell me what it is is how to do it.

regards
Peter
Comment 3 Sebastian Dröge (slomo) 2016-03-08 12:20:20 UTC
You run "git bisect" according to its documentation in a GStreamer checkout, and for each step check if your problem is still there or disappeared. In the end it tells you which commit is the problem.

In any case, I'm not sure what the problem is you're observing. I tried this pipeline. With sync-method=2, nc never receives any data. In 1.7.90 and 1.6.3. With the default sync-method it does. Is this the problem you see? Which versions are you actually testing?

> gst-launch-1.0 -v audiotestsrc !\
>         queue ! audioconvert ! audiorate !\
>         voaacenc bitrate=128000 !\
>         audio/mpeg,mpegversion=4, stream-format=raw !\
>         aacparse ! queue !\
>         muxer. videotestsrc !\
>         queue max-size-bytes=0 leaky=2 max-size-buffers=30 !\
>         videoconvert ! queue !\
>         x264enc bitrate=3499 tune=zerolatency speed-preset=6 key-int-max=60 bframes=0 !\
>         video/x-h264,alignment=au,stream-format=byte-stream,profile=main !\
>         h264parse ! queue !\
>         mpegtsmux name=muxer !\
>         tsparse ! queue !\
>         tcpserversink port=5010 host=127.0.0.1 sync-method=2
Comment 4 Sebastian Dröge (slomo) 2016-03-08 12:30:35 UTC
In any case, the problem I see is that tsmux/tsparse never marks buffers as keyframe except for the very beginning. The problem here is at least the delta unit logic in tsmux, which is completely broken. But there was no change in there since 2012
Comment 5 Tim-Philipp Müller 2016-03-08 12:38:08 UTC
I could swear I fixed this not too long ago..
Comment 6 Peter Maersk-Moller 2016-03-08 12:43:58 UTC
See inline

(In reply to Sebastian Dröge (slomo) from comment #3)
> You run "git bisect" according to its documentation in a GStreamer checkout,
> and for each step check if your problem is still there or disappeared. In
> the end it tells you which commit is the problem.

I'm checking it now. However it's a little complicated as it is probably caused by a relation between at least two modules. Anyway, I'll give it a go.


> In any case, I'm not sure what the problem is you're observing. I tried this
> pipeline. With sync-method=2, nc never receives any data. In 1.7.90 and
> 1.6.3. With the default sync-method it does. Is this the problem you see?
> Which versions are you actually testing?
> 
> > gst-launch-1.0 -v audiotestsrc !\
> >         queue ! audioconvert ! audiorate !\
> >         voaacenc bitrate=128000 !\
> >         audio/mpeg,mpegversion=4, stream-format=raw !\
> >         aacparse ! queue !\
> >         muxer. videotestsrc !\
> >         queue max-size-bytes=0 leaky=2 max-size-buffers=30 !\
> >         videoconvert ! queue !\
> >         x264enc bitrate=3499 tune=zerolatency speed-preset=6 key-int-max=60 bframes=0 !\
> >         video/x-h264,alignment=au,stream-format=byte-stream,profile=main !\
> >         h264parse ! queue !\
> >         mpegtsmux name=muxer !\
> >         tsparse ! queue !\
> >         tcpserversink port=5010 host=127.0.0.1 sync-method=2


And that is probaly not the way to test it as you we have seen many times when using audiotestsrc and videotestsrc since they start with time=0 while the fdsrc and shmsrc with do-timestamp starts at time!=0. And you used voaacenc, although that may not matter.

For testing purpose, does audiotestsrc output the same sort of timestamps as 'fdsrc do-timestamp=1' does if you set timestamp-offset=SOMETHING for audiotestsrc or does audiotestsrc do some magic bus communication behind the scene telling it is starting at zero anyway ? If not, it would be practical for tests if audiotestsrc could start with timestamps similar to shmsrc and fdsrc etc.
Comment 7 Sebastian Dröge (slomo) 2016-03-08 13:03:37 UTC
My point is that the above pipeline does not even work for the initial connection with netcat. It's also all independent of timestamps or similar, the issue is just that nothing is marked as keyframe when going into tcpserversink.


Tim, maybe you forgot to push the fix? :) It looks like the is_delta flag in the muxer is always false when a packet is going to be output.
Comment 8 Tim-Philipp Müller 2016-03-08 19:30:59 UTC
I even added a unit test for it (bug #706872), or so I thought :)
Comment 9 Sebastian Dröge (slomo) 2016-03-09 06:54:43 UTC
For the very first packet the flag is unset, for all following keyframes it is set
Comment 10 Tim-Philipp Müller 2016-03-10 16:13:33 UTC
> For the very first packet the flag is unset, for all following
> keyframes it is set

I can't reproduce that with your pipeline and .. ! fakesink silent=false -v | grep chain | grep -v delta-unit | head -n 5:

188 bytes, dts: none, pts: 0:00:00.000000000, ... flags: 00000400 header
188 bytes, dts: none, pts: 0:00:20.247800453, ... flags: 00000400 header
188 bytes, dts: none, pts: 0:00:56.563809523, ... flags: 00000400 header
188 bytes, dts: none, pts: 0:01:26.796190476, ... flags: 00000400 header
188 bytes, dts: none, pts: 0:01:57.191111111, ... flags: 00000400 header
Comment 11 Tim-Philipp Müller 2016-03-10 16:19:44 UTC
(The huge distance between frames here is because the queue after the videotestsrc was set to be leaky, with videotestsrc is-live=true we get the expected key frame distance of ca. 2 seconds)
Comment 12 Sebastian Dröge (slomo) 2016-03-11 07:21:57 UTC
Indeed, I was too impatient. What is the problem here then?
Comment 13 Sebastian Dröge (slomo) 2016-03-12 10:27:10 UTC
This is the problem here. The delta-unit flag is handled correctly and buffers from tsmux have all correct flags it seems... but tcpserversink handles HEADER buffers differently :) That's what looking at the tsmux output is not useful, but the complete pipeline does not work.

This was already broken in 1.6.0 though, I have no idea why it worked for you in 1.6.3 (it doesn't here).

commit 43621624c87dab8ed5ce98085fc1d3e91ea269a6
Author: Edward Hervey <edward@centricular.com>
Date:   Fri Apr 17 15:35:43 2015 +0200

    mpegtsmux: Carry over GST_BUFFER_FLAG_HEADER
    
    In the same way we do it for the DELTA_UNIT flag
    
    This allows downstream elements to know whether a given mpeg-ts
    packet contains a corresponding HEADER elementary unit
Comment 14 Sebastian Dröge (slomo) 2016-03-12 10:31:30 UTC
And this is the problem here on the other side: https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/tcp/gstmultihandlesink.c?id=df08f5eabe94c1fd95e2f8efe805cadfa2e799a8#n1931

Not sure if something in multihandlesink or mpegtsmux should be changed here. Maybe multihandlesink should only not forward header buffers if the caps contain a "streamheader" field?
Comment 15 Sebastian Dröge (slomo) 2016-03-12 10:49:32 UTC
Created attachment 323751 [details] [review]
multihandlesink: Forward HEADER buffers if the caps don't contain a streamheader field

And consider buffers without DELTA_UNIT flag but HEADER flag as key-units.
Comment 16 Sebastian Dröge (slomo) 2016-03-12 10:53:44 UTC
Comment on attachment 323751 [details] [review]
multihandlesink: Forward HEADER buffers if the caps don't contain a streamheader field

This does not help though, as mpegts caps contain "streamheader". Maybe we should also check if the header buffers are the same in the caps somewhere?

The main problem here is that mpegtsmux considers the sync points as headers, and other elements assume they can ignore headers unless they didn't change or were never forwarded so far.


I think the code in multihandlesink is wrong though. It should always forward all buffers, unless it's going to send exactly the same buffers from the caps at the same time (then it should only send buffers once obviously). What do you think?
Comment 17 Sebastian Dröge (slomo) 2016-03-13 08:41:38 UTC
Also that multihandlesink assumes that a single buffer with the header flag is enough to replace all streamheaders with it seems dubious. Often it's not a single header buffer at all.
Comment 18 Sebastian Dröge (slomo) 2016-03-13 09:00:27 UTC
Created attachment 323782 [details] [review]
multihandlesink: Only don't send HEADER buffers normally if they are actually streamheaders from the caps

And also consider HEADER buffers without DELTA_UNIT flag as sync points. This
fixes sync-mode=2 with mpegtsmux for example, which has no streamheaders but
puts the HEADER flag on its keyframes.
Comment 19 Sebastian Dröge (slomo) 2016-03-13 09:01:39 UTC
Created attachment 323783 [details] [review]
tcp: Remove unused file

It's a copy of multihandlesink, but completely outdated. Let's get rid of it
before it gets even more outdated.
Comment 20 Sebastian Dröge (slomo) 2016-03-13 09:10:47 UTC
Created attachment 323784 [details] [review]
multihandlesink: Only don't send HEADER buffers normally if they are actually streamheaders from the caps

And also consider HEADER buffers without DELTA_UNIT flag as sync points. This
fixes sync-mode=2 with mpegtsmux for example, which has no streamheaders but
puts the HEADER flag on its keyframes.
Comment 21 Sebastian Dröge (slomo) 2016-03-13 09:19:51 UTC
Created attachment 323785 [details] [review]
multihandlesink: Only don't send HEADER buffers normally if they are actually streamheaders from the caps

And also consider HEADER buffers without DELTA_UNIT flag as sync points. This
fixes sync-mode=2 with mpegtsmux for example, which has no streamheaders but
puts the HEADER flag on its keyframes.
Comment 22 Sebastian Dröge (slomo) 2016-03-13 11:57:33 UTC
Created attachment 323786 [details] [review]
multihandlesink: Only don't send HEADER buffers normally if they are actually streamheaders from the caps

And also consider HEADER buffers without DELTA_UNIT flag as sync points. This
fixes sync-mode=2 with mpegtsmux for example, which has no streamheaders but
puts the HEADER flag on its keyframes.
Comment 23 Sebastian Dröge (slomo) 2016-03-13 12:02:12 UTC
Created attachment 323789 [details] [review]
multihandlesink: Remove useless streamheader storage

We don't do anything with it but always get them from the caps anyway, so
stop storing them and having complicated logic around that.
Comment 24 Sebastian Dröge (slomo) 2016-03-14 11:29:30 UTC
commit 65390b5129ce44b0785ec765dbf659249ab59cca
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sun Mar 13 13:59:25 2016 +0200

    multihandlesink: Remove useless streamheader storage
    
    We don't do anything with it but always get them from the caps anyway, so
    stop storing them and having complicated logic around that.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763278

commit 1d4fb48718a2f8e5610c1fe2761640dd0435a9c4
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sun Mar 13 10:51:30 2016 +0200

    multihandlesink: Only don't send HEADER buffers normally if they are actually streamheaders from the caps
    
    And also consider HEADER buffers without DELTA_UNIT flag as sync points. This
    fixes sync-mode=2 with mpegtsmux for example, which has no streamheaders but
    puts the HEADER flag on its keyframes.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763278