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 772468 - basesink: Don't nest prepare/render calls
basesink: Don't nest prepare/render calls
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-10-05 18:56 UTC by Nicolas Dufresne (ndufresne)
Modified: 2016-11-03 19:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
basesink: Don't nest prepare/render calls (1.57 KB, patch)
2016-10-05 18:56 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
basesink: Make sure we never drop the preroll buffer (1.28 KB, patch)
2016-11-03 19:26 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2016-10-05 18:56:54 UTC
During preroll, the prepare/render is no longer called in pair. It's confusing,
in fact, the prepare/preroll pair from the nested into the prepare/render. As
we often use the same function for both prepare and preroll, it's quite
annoying bug.
Comment 1 Nicolas Dufresne (ndufresne) 2016-10-05 18:56:57 UTC
Created attachment 337004 [details] [review]
basesink: Don't nest prepare/render calls

When the first buffer arrives, we endup calling:

  ->prepare()
    ->prepare()
    ->preroll()
  ->render()

This will likely confuse any element using this method. With this patch,
we ensure the preroll take place before the first render prepare() is
called. This will result in:

  ->prepare()
  ->preroll()
  ->prepare()
  ->render()
Comment 2 Matthew Waters (ystreet00) 2016-10-19 06:54:19 UTC
Review of attachment 337004 [details] [review]:

One question.

::: libs/gst/base/gstbasesink.c
@@ +3419,2 @@
     if (G_UNLIKELY (late))
       goto dropped;

Don't we always want to preroll without dropping?

i.e. move the new code above this if?
Comment 3 Nicolas Dufresne (ndufresne) 2016-10-19 14:01:07 UTC
(In reply to Matthew Waters (ystreet00) from comment #2)
> Review of attachment 337004 [details] [review] [review]:
> 
> One question.
> 
> ::: libs/gst/base/gstbasesink.c
> @@ +3419,2 @@
>      if (G_UNLIKELY (late))
>        goto dropped;
> 
> Don't we always want to preroll without dropping?
> 
> i.e. move the new code above this if?

Yes, looks right. It looks like this minor bug existed before this patch.
Comment 4 Sebastian Dröge (slomo) 2016-10-20 09:22:50 UTC
Just include it in here, maybe as another independent commit :)
Comment 5 Nicolas Dufresne (ndufresne) 2016-11-03 19:26:10 UTC
Created attachment 339064 [details] [review]
basesink: Make sure we never drop the preroll buffer

This is cosmetic as 'late' should never be set during preroll (in pause).
Though code may evolve in the future, so this is good for preventing
potential bugs.
Comment 6 Nicolas Dufresne (ndufresne) 2016-11-03 19:28:39 UTC
Attachment 337004 [details] pushed as 5ca63b7 - basesink: Don't nest prepare/render calls
Attachment 339064 [details] pushed as 7c8087f - basesink: Make sure we never drop the preroll buffer