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