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

[Victor Toso]

Hi,

On Fri, Feb 26, 2016 at 04:56:00PM +0100, Francois Gouget wrote:
> 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.

>From my tests, changing transparency of the stream is changing the
height of the video.  That's the bug.  However, note that the bug is on
RHEL6, using sized streams which is disabled by default on RHEL7 and
upstream.

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

Yes, it isn't a fix. What it does is avoiding the crash.
I wouldn't say it is wrong but just that it isn't the proper fix to the
bug.

Still, better then having spice server crashing when watching a video.

> 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

I also would love to have it simplified.

Cheers,
  toso