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

[Francois Gouget]

On Fri, 29 Apr 2016, Christophe Fergeau wrote:
[...]
> > +/* 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?

Yes. The code does not support copying a line that straddles chunks. I 
also have not encountered any case where this happened so I think there 
is no point adding that complexity.


[...]
> > +         /* 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?

Yes, the is_chunk_padded() check guarantees it. I could rename it to 
is_chunk_stride_aligned() to make it clearer.


> > +/* 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

Fixed.


> > +            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.

data_size is the size of the encoded frame. It it exceeds 2GB then 
something is seriously wrong. That said, the SpiceMsgDisplayStreamData 
structures use an uint32_t so I think that's the type to use for 
encode_frame() (and in fact that's what's used after I introduce 
VideoBuffer).


-- 
Francois Gouget <fgouget at codeweavers.com>