[Spice-devel] [PATCH 1/2] server: Remove the width and height parameters of encode_frame()

[Francois Gouget]

On Fri, 26 Feb 2016, Frediano Ziglio wrote:

> > 
> > They always match the size of the source bitmap area:
> > * because for sized frames they are set from the source area,
> > * and because otherwise the initial stream's size is that of regular
> >   frames by definition.
> > Note also that we do not lose any flexibility since the source area
> > parameter can be used to encode a specific bitmap area.
> > 
> 
> In theory this is true. We have however a bug which demonstrate
> that could happen the opposite.
> https://bugzilla.redhat.com/show_bug.cgi?id=1274575.

I'm not convinced by this bug. It does not sound like anyone figured out 
where the wrong size came from which I think is a prerequisite to 
actually fixing the bug.

The 'Application transferred too many/few scanlines' traces are 
generated by libjpeg and they don't necessarily mean that width/height 
does not match the src rect. It could be that the MJPEG encoder does 
not correctly update the libjpeg data when the stream size changes.

In any case the patch attached to the bug is clearly wrong: 
mjpeg-encoder's encode_frame() function is the wrong place to suddenly 
realize that we're not sure of the size of the frame we're supposed to 
encode. This is sweeping the bug under the rug, not fixing it.

I am however concerned that this width/height vs. rect thing is embedded 
in many different places: SpiceMsgDisplayStreamCreate (width/height 
vs. dest), Stream (width/height vs. dest_area), ItemTrace (width/height 
vs. dest_area).

If there really is a reason for having both, it is, as far as I can 
tell, totally undocumented. There is also nothing that says when to use 
one, and when to use the other. Without that information it's obviously 
impossible to write a video encoder or anything that manipulates the 
frames.


-- 
Francois Gouget <fgouget at codeweavers.com>