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

[Christophe Fergeau]

On Tue, May 03, 2016 at 03:58:58PM +0200, Francois Gouget wrote:
> 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.

Ah yeah definitely not, but I got misled by 'padding', and thought this
was about a chunk with:
xxxxxxxxxxxxxxxxxxxxx......
with the 'x's data we are interested in, and the '.' some added padding,
and I was asking why we would not be able to handle such cases. But
with your added explanation, these '.' would be counted in
'bitmap->stride' anyway, so this can't happen. I was definitely not
suggesting handling lines split over multiple chunks.

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

Hmm, this guarantees we have at least bitmap->stride bytes, which
is (assumed to be?) bigger than stream_stride. Is there an explicit
check/reason that bitmap->stride is bigger than stream_stride?
Also, is there anything preventing chunks->chunk[index].len to be 0 in
is_chunk_padded()? is_chunk_stride_aligned() is probably a better name
for it as you suggest. I'd add a comment saying that we don't support
lines split over several chunks.

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

Ah right, this changes when VideoBuffer is added. I personnally would
have used a gsize field and done the truncation when the message is
marshalled, but I can live with the uint32_t field added in the
VideoBuffer patch, and since the latter patch changes that part of this
patch, not much point in changing this one.

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/20160503/effe9d55/attachment.sig>