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@xxxxxxxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel