[Spice-devel] [PATCH v6 11/26] server: Let the video encoder manage the compressed buffer

[Christophe Fergeau]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151022/a849fabe/attachment-0001.sig>