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