[FFmpeg-devel] [PATCH] pulse: set default frame_size to 4608

[Michael Niedermayer]

On Fri, Jan 03, 2014 at 01:51:25AM +0100, Lukasz M wrote:
> On 3 January 2014 01:21, Federico Simoncelli
> <federico.simoncelli at gmail.com>wrote:
> 
> > On Thu, Jan 2, 2014 at 11:26 PM, Michael Niedermayer <michaelni at gmx.at>
> > wrote:
> > > On Thu, Jan 02, 2014 at 04:36:45PM +0100, Federico Simoncelli wrote:
> > >> Given the current defaults (channels = 2, sample_rate = 48000) the
> > >> frame_size is changed to 4608 in order to obtain whole numbers for
> > >> frame_duration (both 16 and 24 bits_per_sample).
> > >>
> > >>  frame_duration = (frame_size * 1000000 * 8) /
> > >>                   (sample_rate * channels * bits_per_sample)
> > >>
> > >> A message has been added to warn the user when the frame duration
> > >> is not an integer.
> > >
> > > the timebase should be changed to a multiple of the sample rate
> > > see the avpriv_set_pts_info() call
> > > that avoids the problem of the durations being not exactly
> > > representable
> >
> > If I understand correctly what you're suggesting is:
> >
> > avpriv_set_pts_info(st, 64, 1, pd->sample_rate);
> >
> > and it will generate non-representable timebases:
> >
> > 1/48 = .020833...
> > 1/44.1 = .0226757
> >
> > > and the frame_size either should be "redefined" to mean samples
> > > across all channels so that 8bit samples with 3 channels and a
> > > frame size of 1024 means 3072 bytes
> >
> > I thought that producing a warning was good enough (consider also the
> > answer in the next paragraph).
> > Are you sure you want break the backward compatibility of the
> > frame_size parameter?
> >
> > > cutting frames between channels is quiet bad so a frame size
> > > in bytes is problematic but if its kept in bytes then a default of
> > > 3360 should avoid the fractional durations with the changed timebase
> >
> > I think that my patch handles the frames cut between channels case. At
> > the denominator there's "channels":
> >
> > frame_duration = (frame_size * 1000000 * 8) /
> >                  (sample_rate * channels * bits_per_sample)
> >
> 
> I believe Michalel meant that your solution is not generic. It works
> perfectly for default option and leaves a warning for one of 2 issues that
> may occur on user specific config:
> - duration of the frame is not multiple of channels * bytes per sample
> (covered by warning)
> - frame duration cannot be presented as multiple of straem time base
> 
> My proposition is to calculate frame duration unless it is provided with
> formula
> 
> frame_duration = channels * bytes per frame * X // X is magic number you
> need to calculate to make frame duration 1024 + extra padding to contain
> all bytes per sample across all channels
> and you set time_base of the stream
> avpriv_set_pts_info(st, 64, X, pd->sample_rate); //X here is the same value
> as above
> this will produce pts: 0, 1 ,2 ,3...
> 
> in options you make frame_duration to 0 and if it is not set then calculate
> as above.
> 

> You may provide error as you did when frame duration is provided and it
> cannot be divided by channels * bytes per frame and return with EINVAL
> error.
> In case it can be divided then set

> avpriv_set_pts_info(st, 64, duration / (channels * bytes per frame),
> pd->sample_rate);

a timebase that has sample precission or something on that order
is needed to not loose precission of the value from
pa_simple_get_latency()

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140103/adec2b9c/attachment.asc>