[Spice-devel] [spice v2 2/2] utils: Remove the LL suffix from NSEC_PER_SEC
Frediano Ziglio
fziglio at redhat.com
Thu Jun 13 11:47:00 UTC 2019
>
> On 6/13/19 1:23 PM, Frediano Ziglio wrote:
> >>
> >> This constant fits in a 32 bit signed integer so it does not need the
> >> suffix. However some of the derived constants don't so use an uint64_t
> >> cast to avoid the long vs long long confusion (such as in print
> >> statements).
> >> Also some of the expressions these constants are used in may overflow so
> >> perform the appropriate casts in those locations. This makes it clearer
> >> that these operations are 64 bit.
>
> Hi,
>
> Wouldn't it be simpler to replace the LL suffix with
> a prefix of (uint64_t) ?
>
> -#define NSEC_PER_SEC 1000000000LL
> +#define NSEC_PER_SEC (uint64_t)1000000000
>
> That way there is no worry about overflows.
>
> Uri.
>
You could also use the INT64_C macro for this, but Francois is trying to do
the opposite, not have the constant 64 bit in order to make the compiler able
to decide to use 32 bit arithmetic if it wants.
Note that "LL" is signed while uint64_t is unsigned, this could change some
math causing some nasty behaviour, I remember once I changed some code "easily"
like that and I got a nice buffer overflow for free.
>
> >>
> >> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> >> ---
> >>
> >> v2: Cast COMMON_CLIENT_TIMEOUT to an int64_t instead of an uint64_t.
> >
> > I would personally use signed for all timeouts, specifically also
> > DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT, MJPEG_WARMUP_TIME and
> > RED_STREAM_INPUT_FPS_TIMEOUT.
> >
> > Beside that patch is good.
> >
> >
> >> v1:
> >> https://lists.freedesktop.org/archives/spice-devel/2019-May/049193.html
> >>
> >> server/common-graphics-channel.h | 2 +-
> >> server/dcc.h | 2 +-
> >> server/gstreamer-encoder.c | 2 +-
> >> server/mjpeg-encoder.c | 2 +-
> >> server/utils.h | 4 ++--
> >> server/video-stream.c | 4 ++--
> >> server/video-stream.h | 2 +-
> >> 7 files changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/server/common-graphics-channel.h
> >> b/server/common-graphics-channel.h
> >> index d23f0c695..cccd24d44 100644
> >> --- a/server/common-graphics-channel.h
> >> +++ b/server/common-graphics-channel.h
> >> @@ -27,7 +27,7 @@ G_BEGIN_DECLS
> >>
> >> bool common_channel_client_config_socket(RedChannelClient *rcc);
> >>
> >> -#define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)
> >> +#define COMMON_CLIENT_TIMEOUT (((int64_t)NSEC_PER_SEC) * 30)
> >>
> >> #define TYPE_COMMON_GRAPHICS_CHANNEL common_graphics_channel_get_type()
> >>
> >> diff --git a/server/dcc.h b/server/dcc.h
> >> index 76c078bf5..da8697762 100644
> >> --- a/server/dcc.h
> >> +++ b/server/dcc.h
> >> @@ -66,7 +66,7 @@ GType display_channel_client_get_type(void)
> >> G_GNUC_CONST;
> >> #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
> >> #define CLIENT_PALETTE_CACHE_SIZE 128
> >>
> >> -#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (NSEC_PER_SEC * 10)
> >> +#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (((uint64_t)NSEC_PER_SEC) *
> >> 10)
> >> #define DISPLAY_CLIENT_RETRY_INTERVAL 10000 //micro
> >>
> >> /* Each drawable can refer to at most 3 images: src, brush and mask */
> >> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> >> index 5f39ed877..91dd475ac 100644
> >> --- a/server/gstreamer-encoder.c
> >> +++ b/server/gstreamer-encoder.c
> >> @@ -556,7 +556,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
> >> *encoder)
> >> /* Figure out how many frames to drop to not exceed the current bit
> >> rate.
> >> * Use nanoseconds to avoid precision loss.
> >> */
> >> - uint64_t delay_ns = -encoder->vbuffer_free * 8 * NSEC_PER_SEC /
> >> encoder->bit_rate;
> >> + uint64_t delay_ns = ((uint64_t)-encoder->vbuffer_free) * 8 *
> >> NSEC_PER_SEC / encoder->bit_rate;
> >> uint32_t drops = (delay_ns + period_ns - 1) / period_ns; /* round up
> >> */
> >> spice_debug("drops=%u vbuffer %d/%d", drops, encoder->vbuffer_free,
> >> encoder->vbuffer_size);
> >> diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
> >> index 4a02e7c8b..d22e7d18b 100644
> >> --- a/server/mjpeg-encoder.c
> >> +++ b/server/mjpeg-encoder.c
> >> @@ -65,7 +65,7 @@ static const int
> >> mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
> >> * are not necessarily related to mis-estimation of the bit rate, and we
> >> would
> >> * like to wait till the stream stabilizes.
> >> */
> >> -#define MJPEG_WARMUP_TIME (NSEC_PER_SEC * 3)
> >> +#define MJPEG_WARMUP_TIME (((uint64_t)NSEC_PER_SEC) * 3)
> >>
> >> /* The compressed buffer initial size. */
> >> #define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
> >> diff --git a/server/utils.h b/server/utils.h
> >> index 54bc9d490..4bbd45dff 100644
> >> --- a/server/utils.h
> >> +++ b/server/utils.h
> >> @@ -52,7 +52,7 @@ static inline int test_bit(int index, uint32_t val)
> >>
> >> typedef int64_t red_time_t;
> >>
> >> -#define NSEC_PER_SEC 1000000000LL
> >> +#define NSEC_PER_SEC 1000000000
> >> #define NSEC_PER_MILLISEC 1000000
> >> #define NSEC_PER_MICROSEC 1000
> >>
> >> @@ -62,7 +62,7 @@ static inline red_time_t
> >> spice_get_monotonic_time_ns(void)
> >> struct timespec time;
> >>
> >> clock_gettime(CLOCK_MONOTONIC, &time);
> >> - return NSEC_PER_SEC * time.tv_sec + time.tv_nsec;
> >> + return ((uint64_t)NSEC_PER_SEC) * time.tv_sec + time.tv_nsec;
> >> }
> >>
> >> #define MSEC_PER_SEC 1000
> >> diff --git a/server/video-stream.c b/server/video-stream.c
> >> index 4ac25af8b..7a2dca7dd 100644
> >> --- a/server/video-stream.c
> >> +++ b/server/video-stream.c
> >> @@ -415,8 +415,8 @@ static void
> >> display_channel_create_stream(DisplayChannel
> >> *display, Drawable *dra
> >> * the nearest integer, for instance 24 for 23.976.
> >> */
> >> uint64_t duration = drawable->creation_time -
> >> drawable->first_frame_time;
> >> - if (duration > NSEC_PER_SEC * drawable->frames_count / MAX_FPS) {
> >> - stream->input_fps = (NSEC_PER_SEC * drawable->frames_count +
> >> duration / 2) / duration;
> >> + if (duration > ((uint64_t)NSEC_PER_SEC) * drawable->frames_count /
> >> MAX_FPS) {
> >> + stream->input_fps = (((uint64_t)NSEC_PER_SEC) *
> >> drawable->frames_count + duration / 2) / duration;
> >> } else {
> >> stream->input_fps = MAX_FPS;
> >> }
> >> diff --git a/server/video-stream.h b/server/video-stream.h
> >> index 46b076fd7..6c18d00f7 100644
> >> --- a/server/video-stream.h
> >> +++ b/server/video-stream.h
> >> @@ -34,7 +34,7 @@
> >> #define RED_STREAM_GRADUAL_FRAMES_START_CONDITION 0.2
> >> #define RED_STREAM_FRAMES_RESET_CONDITION 100
> >> #define RED_STREAM_MIN_SIZE (96 * 96)
> >> -#define RED_STREAM_INPUT_FPS_TIMEOUT (NSEC_PER_SEC * 5)
> >> +#define RED_STREAM_INPUT_FPS_TIMEOUT (((uint64_t)NSEC_PER_SEC) * 5)
> >> #define RED_STREAM_CHANNEL_CAPACITY 0.8
> >> /* the client's stream report frequency is the minimum of the 2 values
> >> below
> >> */
> >> #define RED_STREAM_CLIENT_REPORT_WINDOW 5 // #frames
> >
> > Frediano
More information about the Spice-devel
mailing list