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 721035 - tsdemux: fails to seek on the attached file (regression)
tsdemux: fails to seek on the attached file (regression)
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal blocker
: 1.3.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 710118 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-12-25 11:47 UTC by Julien Isorce
Modified: 2014-06-10 12:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mpegtspacketizer: Fix ts_to_offset beyond last observed PCR (999 bytes, patch)
2013-12-29 09:01 UTC, Edward Hervey
committed Details | Review
!mpegtspacketizer: go to next group if pcr is 0 and querypcr (3.16 KB, patch)
2013-12-30 17:05 UTC, Julien Isorce
rejected Details | Review

Description Julien Isorce 2013-12-25 11:47:55 UTC
* material: http://people.collabora.com/~julien/videos/big_buck_bunny.tar.gz

* steps to reproduce:
Run gst-launch-1.0 filesrc location=big_buck_bunny.ts  ! decodebin ! navseek ! xvimagesink  then press "right arrow" while keeping mouse pointer over video window

* actual result:
fails here http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/mpegtsdemux/mpegtspacketizer.c#n2266 with GStreamer-CRITICAL **: _gst_util_uint64_scale: assertion 'denom != 0' failed

* remarks:
here "nextgroup == prevgroup" so I can see GST_DEBUG ("In group")

lastpcr is equal to firstpcr because prevgroup->values[prevgroup->last_value].pcr is 0 
So the thrid parameter of "gst_util_uint64_scale (querypcr - firstpcr,
lastoffset - firstoffset, lastpcr - firstpcr);" is 0

I wonder if the problem would be that "prevgroup->values[prevgroup->last_value].pcr" is not updated correctly

* other infos:
Note that before this commit
http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/gst/mpegtsdemux/mpegtspacketizer.c?id=2762ead5ef2a16885fbb6918dae9b8df836a361f  there was not the problem but the seek was not working so far
Comment 1 Tim-Philipp Müller 2013-12-28 16:46:22 UTC
Reproduced, and regression from 1.2.
Comment 2 Edward Hervey 2013-12-29 08:48:57 UTC
Confirmed.

It's the case where you ask for a byte position for a time greater than the last observed time position.
Comment 3 Edward Hervey 2013-12-29 09:01:58 UTC
Created attachment 264992 [details] [review]
mpegtspacketizer: Fix ts_to_offset beyond last observed PCR

The requested TS might be beyond the last observed PCR. In order to calculate
a coherent offset, we need to use the last and previous-to-last groups.
Comment 4 Julien Isorce 2013-12-30 17:03:26 UTC
Comment on attachment 264992 [details] [review]
mpegtspacketizer: Fix ts_to_offset beyond last observed PCR

tested and it works here
Comment 5 Julien Isorce 2013-12-30 17:05:03 UTC
Created attachment 265047 [details] [review]
!mpegtspacketizer: go to next group if pcr is 0 and querypcr

Though with http://people.collabora.com/~julien/videos/big_buck_bunny_1080p_ts.tar.gz
if you do:

gst-launch-1.0 filesrc location=big_buck_bunny.ts  ! decodebin ! navseek=1 !
xvimagesink
and press "left arrow" it will also raise "GStreamer-CRITICAL **: _gst_util_uint64_scale: assertion 'denom != 0'
failed"

So see attached patch but really not sure about that. Maybe I only did a workaround without really fixing the underlying problem.
Comment 6 Julien Isorce 2013-12-30 17:07:41 UTC
Also note that if you keep "left arrow", there are a lot of "GStreamer-CRITICAL **: gst_segment_do_seek: assertion 'segment->format == format' failed" but I guess it's another problem
Comment 7 Edward Hervey 2013-12-31 06:22:25 UTC
Eh, ok. So I guess we have the opposite problem also :) Will look into it later.

If only we had some tool to automate those seeking tests ...
Comment 8 Edward Hervey 2014-02-20 14:53:45 UTC
Comment on attachment 264992 [details] [review]
mpegtspacketizer: Fix ts_to_offset beyond last observed PCR

This one was 100% certain
Comment 9 Edward Hervey 2014-02-20 14:54:30 UTC
Julien, do you still get the issues without your patch ?
Comment 10 Edward Hervey 2014-02-20 15:22:53 UTC
*** Bug 710118 has been marked as a duplicate of this bug. ***
Comment 11 Arnaud Vrac 2014-04-14 11:10:30 UTC
There's also another regression similar to this one compared to 1.2, seeking does not work anymore in push mode, for two reasons:

 - you now need two observation groups to get a duration estimate
 - you can't seek beyond the last seen PCR

In 1.2 it was possible to estimate a byte position from a timestamp based only on the PCR seen so far, which is not possible anymore.
Comment 12 Edward Hervey 2014-06-10 12:33:59 UTC
(In reply to comment #11)
> There's also another regression similar to this one compared to 1.2, seeking
> does not work anymore in push mode, for two reasons:
> 
>  - you now need two observation groups to get a duration estimate
>  - you can't seek beyond the last seen PCR
> 
> In 1.2 it was possible to estimate a byte position from a timestamp based only
> on the PCR seen so far, which is not possible anymore.

This is now fixed with this commit:
commit 8e28f335f40b368a8a545283f4951f992102c4f8
Author: Edward Hervey <edward@collabora.com>
Date:   Tue Jun 10 14:27:01 2014 +0200

    mpegtspacketizer: Fix duration evaluation in push mode
    
    When working in push mode, we need to be able to evaluate the duration
    based on a single group of observations.
    
    To do that we use the current group values
Comment 13 Edward Hervey 2014-06-10 12:53:39 UTC
(In reply to comment #6)
> Also note that if you keep "left arrow", there are a lot of "GStreamer-CRITICAL
> **: gst_segment_do_seek: assertion 'segment->format == format' failed" but I
> guess it's another problem

That warning was due to some leftover (unused) code.

Reported use-cases now work. Closing bug.

commit 0020a9344d53a5298c486817171f3aecf6c55249
Author: Edward Hervey <edward@collabora.com>
Date:   Tue Jun 10 14:50:10 2014 +0200

    tsdemux: Remove unused variable
    
    The seeksegment was no longer used since the switch to calculating segments
    when we see data.