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

[Frediano Ziglio]

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

Actually Francois I think you are the most expert on this part of code.
I spent lot of time last week trying to understand another bug on streaming
and I just came out with some colored videos.
And as the author (Victor) of the patch is a "proposal patch".

> 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 think this is not hard (I hope) to check.

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

As I said I think you are the expert here, if you don't figure it out
means that's quite hard to understand.
Personally I don't understand the "sized" thing. If mjpeg is just a sequence
of jpeg and if we can't have a single stream (I never saw a video changing
frame size every frame...) looks like we just compress the images we are
sending with jpeg... just using crazy protocol and code!
But as I said I'm far from full comprehension but I won't be surprised that's
something really wrong.

> 
> --
> Francois Gouget <fgouget at codeweavers.com>
> 

Frediano