[Spice-devel] [spice v13 03/29] server: Add a GStreamer 1.0 MJPEG video encoder and use it by default

[Christophe Fergeau]

Hey,

A few small comments/questions

On Tue, Apr 19, 2016 at 09:48:19AM +0200, Francois Gouget wrote:
> +/* A helper for the *_copy() functions */
> +static int is_chunk_padded(const SpiceBitmap *bitmap, uint32_t index)
> +{
> +    SpiceChunks *chunks = bitmap->data;
> +    if (chunks->chunk[index].len % bitmap->stride != 0) {
> +        spice_warning("chunk %d/%d is padded, cannot copy", index, chunks->num_chunks);
> +        return TRUE;
> +    }
> +    return FALSE;
> +}
> +
> +/* A helper for push_raw_frame() */
> +static inline int line_copy(SpiceGstEncoder *encoder, const SpiceBitmap *bitmap,
> +                            uint32_t chunk_offset, uint32_t stream_stride,
> +                            uint32_t height, uint8_t *buffer)
> +{
> +     uint8_t *dst = buffer;
> +     SpiceChunks *chunks = bitmap->data;
> +     uint32_t chunk_index = 0;
> +     for (int l = 0; l < height; l++) {
> +         /* We may have to move forward by more than one chunk the first
> +          * time around.
> +          */
> +         while (chunk_offset >= chunks->chunk[chunk_index].len) {
> +             if (is_chunk_padded(bitmap, chunk_index)) {
> +                 return FALSE;
> +             }

Does this have to fail since we are doing a line by line copy?

> +             chunk_offset -= chunks->chunk[chunk_index].len;
> +             chunk_index++;
> +         }
> +
> +         /* Copy the line */
> +         uint8_t *src = chunks->chunk[chunk_index].data + chunk_offset;
> +         memcpy(dst, src, stream_stride);

Are we guaranteed that we'll have at least 'stream_stride' bytes in the
chunk?

> +/* A helper for spice_gst_encoder_encode_frame() */
> +static int pull_compressed_buffer(SpiceGstEncoder *encoder,
> +                                  uint8_t **outbuf, size_t *outbuf_size,
> +                                  int *data_size)
> +{
> +    spice_return_val_if_fail(outbuf && outbuf_size, VIDEO_ENCODER_FRAME_UNSUPPORTED);
> +
> +    GstSample *sample = gst_app_sink_pull_sample(encoder->appsink);
> +    if (sample) {
> +        GstMapInfo map;
> +        GstBuffer *buffer = gst_sample_get_buffer(sample);
> +        if (buffer && gst_buffer_map(buffer, &map, GST_MAP_READ)) {
> +            int size = gst_buffer_get_size(buffer);

This returns a gsize

> +            if (!*outbuf || *outbuf_size < size) {
> +                free(*outbuf);
> +                *outbuf = spice_malloc(size);
> +                *outbuf_size = size;
> +            }
> +            /* TODO Try to avoid this copy by changing the GstBuffer handling */
> +            memcpy(*outbuf, map.data, size);
> +            *data_size = size;

So I'd make this a gsize/size_t as well. encode_frame external API uses
an int* for historical reasons, I'm fine with keeping the int* in
spice_gst_encoder_encode_frame for now, but I'd prefer that the
information loss happens in encode_frame rather than being already lost
in pull_compressed_buffer.


Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160429/397130cb/attachment.sig>