[Spice-devel] [spice] gstreamer-encoder: Return the average frame size as a 32 bit int

[Frediano Ziglio]

> 
> On Thu, 16 May 2019, Frediano Ziglio wrote:
> [...]
> > > @@ -520,7 +520,7 @@ static uint32_t
> > > get_min_playback_delay(SpiceGstEncoder
> > > *encoder)
> > >       * an I frame) and an average frame. This also takes into account
> > >       the
> > >       * frames dropped by the encoder bit rate control.
> > >       */
> > > -    uint64_t size = get_maximum_frame_size(encoder) +
> > > get_average_frame_size(encoder);
> > > +    uint32_t size = get_maximum_frame_size(encoder) +
> > > get_average_frame_size(encoder);
> > >      uint32_t send_time = MSEC_PER_SEC * size * 8 / encoder->bit_rate;
> > >  
> > 
> > Here you have 8000 * 2 * frame_size so could overflow uint32_t with
> > frame_size >= ~256kb.
> 
> Right. It's confusing that NSEC_PER_SEC and NSEC_PER_MILLISEC are LL
> constants but not NSEC_PER_MICROSEC and MSEC_PER_SEC. Should that be
> changed?
> 
> Since they are all less than or equal to one billion, should they just
> be basic constants (which would avoid the %lld vs. %ld confusion
> whenever they are used in a trace) or should they all be LL constants to
> avoid unexpected overflows.
> 

Well, on one side you avoid the usage of big number, on the other side
you are proposing to use all 64 bit at least.
I would avoid to move to all 64 bit, even if more and more machines are
64 bit could be still fast to stay on 32 bit, compilers, even on 64 bit
can do some optimizations using 32 bit arithmetic.

> 
> > I agree get_average_frame_size can safely returns uint32_t but you should
> > change above line to
> > 
> >    uint32_t send_time = (uint32_t) ((uint64_t) (MSEC_PER_SEC * 8) * size /
> >    encoder->bit_rate);
> > 
> > or leave size uint64_t.
> 
> I would be ok with that too. It's mostly having get_average_frame_size()
> return an uint64_t when get_maximum_frame_size() returns an uint32_t
> that was bothering me.
> 

Yes, it makes sense. Maybe adding a note if forcing conversion why.

Frediano