On Wed, Oct 14, 2015 at 05:33:38PM +0200, Francois Gouget wrote:
> This way the video encoder is not forced to use malloc()/free().
> This also allows more flexibility in how the video encoder manages the
> buffer which allows for a zero-copy implementation in both video
> encoders.
> The current implementations also ensure that there is no reallocation
> of the VideoBuffer structure.
Looks nicer this way, though is the last sentence still relevant? ('no
reallocation of the VideoBuffer structure').
One small comment below
> diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
> index 74ddf0b..d7f9979 100644
> --- a/server/mjpeg_encoder.c
> +++ b/server/mjpeg_encoder.c
> @@ -70,6 +70,9 @@ static const int mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
> */
> #define MJPEG_WARMUP_TIME 3000LL // 3 sec
>
> +/* The compressed buffer initial size. */
> +#define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
> +
> enum {
> MJPEG_QUALITY_EVAL_TYPE_SET,
> MJPEG_QUALITY_EVAL_TYPE_UPGRADE,
> @@ -154,6 +157,11 @@ typedef struct MJpegEncoderRateControl {
> uint64_t warmup_start_time;
> } MJpegEncoderRateControl;
>
> +typedef struct MJpegVideoBuffer {
> + VideoBuffer base;
> + size_t maxsize;
> +} MJpegVideoBuffer;
> +
> typedef struct MJpegEncoder {
> VideoEncoder base;
> uint8_t *row;
> @@ -181,6 +189,22 @@ static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
> uint64_t byte_rate,
> uint32_t latency);
>
> +static void mjpeg_buffer_free(VideoBuffer *video_buffer)
> +{
> + MJpegVideoBuffer *buffer = (MJpegVideoBuffer*)video_buffer;
> + free(buffer->base.data);
> + free(buffer);
> +}
> +
> +static MJpegVideoBuffer* create_mjpeg_video_buffer(void)
> +{
> + MJpegVideoBuffer *buffer = spice_new0(MJpegVideoBuffer, 1);
> + buffer->base.free = &mjpeg_buffer_free;
> + buffer->maxsize = MJPEG_INITIAL_BUFFER_SIZE;
> + buffer->base.data = malloc(buffer->maxsize);
I think some handling of malloc() returning NULL is missing here.
Maybe just
if (*buffer->base.data == NULL)
ERREXIT1(cinfo, JERR_OUT_OF_MEMORY, 10);
?
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel