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

[Lukasz M]

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);

I hope I didn't make mistake :)

Regards