Re: gst-plugins-good: Revert "Pulsesink: Allow chunks up to bufsize instead of segsize"

[René Stadler]

On Fri, Apr 8, 2011 at 3:35 PM, Sebastian Dröge <[hidden email]> wrote:

Module: gst-plugins-good
Branch: master
Commit: 11bcac7c9015c4db85514e7837cbd2d0f47b9ff1
URL:    http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=11bcac7c9015c4db85514e7837cbd2d0f47b9ff1

Author: Sebastian Dröge <[hidden email]>
Date:   Fri Apr  8 14:35:04 2011 +0200

Revert "Pulsesink: Allow chunks up to bufsize instead of segsize"

This reverts commit 1e2c1467ae042a3c6bb1a6bc0c07aeff13ec5edb.

The commit causes pulsesink to ignore the latency-time baseaudiosink property.

[...] 

Hi,

can you elaborate a bit on this? Strictly speaking, it's not true that the property is ignored without the revert, since segsize/latency-time is still passed to PA's minreq attribute. Since that is the minimum amount of free space that must be available before pulse calls us for more data, that should still have a huge effect (if not, there is something else wrong in pulsesink).

I haven't measured the wakeup behavior and power consumption of pulsesink in a while, so I'm wondering if you have some up-to-date empirical data that prompted you to this revert :)

Regards,

--René

_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

[pl bossart]

> Hi,
> can you elaborate a bit on this? Strictly speaking, it's not true that the
> property is ignored without the revert, since segsize/latency-time is still
> passed to PA's minreq attribute. Since that is the minimum amount of free
> space that must be available before pulse calls us for more data, that
> should still have a huge effect (if not, there is something else wrong in
> pulsesink).
> I haven't measured the wakeup behavior and power consumption of pulsesink in
> a while, so I'm wondering if you have some up-to-date empirical data that
> prompted you to this revert :)

Agree with Rene. The notion of 'latency-time' doesn't make sense here
anyway, this parameter only corresponds to the buffer size send to
pulseaudio and has nothing to do with latency. With the latency
adjustment flag there is no direct relationship between latency-time
and buffer-time.
-Pierre
_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

[Sebastian Dröge-7]

On Fri, 2011-04-08 at 21:02 +0300, René Stadler wrote:

> On Fri, Apr 8, 2011 at 3:35 PM, Sebastian Dröge
> <[hidden email]> wrote:
>         Module: gst-plugins-good
>         Branch: master
>         Commit: 11bcac7c9015c4db85514e7837cbd2d0f47b9ff1
>         URL:
>          http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=11bcac7c9015c4db85514e7837cbd2d0f47b9ff1
>        
>         Author: Sebastian Dröge <[hidden email]>
>         Date:   Fri Apr  8 14:35:04 2011 +0200
>        
>         Revert "Pulsesink: Allow chunks up to bufsize instead of
>         segsize"
>        
>         This reverts commit 1e2c1467ae042a3c6bb1a6bc0c07aeff13ec5edb.
>        
>         The commit causes pulsesink to ignore the latency-time
>         baseaudiosink property.
> [...]
>
>
> Hi,
>
>
> can you elaborate a bit on this? Strictly speaking, it's not true that
> the property is ignored without the revert, since segsize/latency-time
> is still passed to PA's minreq attribute. Since that is the minimum
> amount of free space that must be available before pulse calls us for
> more data, that should still have a huge effect (if not, there is
> something else wrong in pulsesink).

If a decoder passes large buffers to pulsesink these large buffers will
be written do PA instead of smaller, latency-time-sized parts of the
buffer.

> I haven't measured the wakeup behavior and power consumption of
> pulsesink in a while, so I'm wondering if you have some
> up-to-date empirical data that prompted you to this revert :)

I don't but see the discussion here with Stefan:
https://bugzilla.gnome.org/show_bug.cgi?id=641072

I've reverted the patch yesterday because the conclusion seems to be
that this should be fixed elsewhere and Stefan apparently just forgot to
revert it. I didn't want this to be in the release if it isn't correct
and could cause problems. Nonetheless I agree that something has to be
done to decrease power consumption and wakeups, let's continue that
discussion in the bugreport.


Oh and btw, the patch did cause real problems :) See this bug:
https://bugzilla.gnome.org/show_bug.cgi?id=646999

The change caused underuns, at least with WMA but probably with other
formats too. (Note: WMA frames are very large compared to MP3, e.g.
115200 bytes vs. 9216 bytes in one of my testcases)


_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

[René Stadler]

2011/4/9 Sebastian Dröge <[hidden email]>

[...]

I've reverted the patch yesterday because the conclusion seems to be
that this should be fixed elsewhere and Stefan apparently just forgot to
revert it. I didn't want this to be in the release if it isn't correct
and could cause problems. Nonetheless I agree that something has to be
done to decrease power consumption and wakeups, let's continue that
discussion in the bugreport.

[...]

That all makes sense. The problem for me is that I'm not constantly aware of each and every single bug report. That's why it's important to include such information in the commit message for a revert :)

--René

_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel