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

[Victor Toso]

Hi,

On Fri, Feb 26, 2016 at 09:39:49PM +0100, Victor Toso wrote:
> 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.

I forgot to reply to this :)

As the transparency was changing the height of the stream (wrongly), it
was entering in a case where:
- on increasing the transparency:
  mjpeg was created with smaller stream size as we were trying to encode
  triggering the too many scanlines.
- on decreasing the transparency:
  mjepg was  created with bigger stream size as we were trying to encode
  triggering the too few scanlines and exit.

I tried a few things here to avoid such situations but the proposal
patch is the simpler one and it does not seem to cause bigger issues.

Trying to fix the actual change of height due the alpha bit... if you
have any idea how, I would love to know!

Thanks again for this discussion, I really want the stream bits on
server to be improved.

Cheers,
  toso

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