On 2/10/12 11:15 AM, Pierre Ossman wrote:
>> You mean this code?
>>
>> if (ig->willTransform()) {
>> ig->translatePixels(pixels, &solidColor, 1);
>> pixels = (PIXEL_T *)&solidColor;
>> }
>>
>> I don't follow. As you can see, it changes the value of the "pixels"
>> pointer so that it points at the local "solidColor" variable and no
>> longer at the framebuffer, so the framebuffer is not modified. I can't
>> see any other place in the code where the framebuffer was being modified.
>
> But ig->willTransform() can return false, which is even the common case.
I have confirmed that 4841 is not correct. The reason why is that it
assumes that JPEG will always be encoded from the raw pixel buffer, but
that is not always the case. JPEG is only encoded from the raw buffer
whenever no pixel translation is taking place. Otherwise, it will
encode from the translated buffer. The reason for this is so we don't
end up doing pixel translation twice. If, for instance, the server is
16-bpp but the client is 32-bpp, then JPEG can be encoded directly from
the translated buffer instead of having to re-translate the pixels from
16-bit to 24-bit within the JPEG compressor.
This is also why the if(grayScaleJPEG) test has to be in both logical
branches. The way 4841 was written, grayscale mode would not work
properly if JPEG was being compressed from a translated pixel buffer.
Further, this is also why the encodeJpegRect*() routines use clientpf
instead of serverpf (4841 broke this as well.) What's particularly
annoying is that a quick 'svn log' would reveal that the code structure
used by r4841 was already determined to be faulty and fixed by r4642.
4851 and 4852 fixes the regression in both trunk and 1.2, as well as
fixing the initial issue that prompted 4841. I have re-tested at the
low level to ensure that performance has not been affected by this and
that the datastream is byte-accurate relative to r4840.
At this point, I'm not going to lecture anymore regarding checking in
potentially destabilizing code post-beta, but I will beg and plead again
that you (a) treat the Tight encoder as a separate piece of
infrastructure and assume that it will require manual low-level
regression and performance testing every time it is modified and (b)
file bug reports on any issues related to it. When I've made this
request in the past, I've been accused of trying to stifle open
development in this project, but I assure you that that's not the case.
I assure you that other open source projects like Mozilla make the same
requirements of their image codec infrastructure (much stricter ones,
typically.) Any complicated image encoder is a delicate beast requiring
extensive testing when it is modified. The difference between our
project and others is that others typically automate that process, and
that's something TigerVNC really should do as well. I'm not
volunteering to make that happen, but until/unless it does, we can't
just modify the encoder and expect that it is regression-proof because
the high-level tests don't reveal anything. Every potentially
regressive modification to that code must be tested manually at the low
level.
Regarding formatting, my coding style favors preservation of vertical
space, because studies (as well as my own experience) have shown that
code is more readable when you can see more of its logical flow at once.
However, I compromised with you on that in 4851 and 4852, preserving
some of your formatting changes from 4841 that made sense. I also
improved the comments to better describe what the code is doing
vis-a-vis pixel translation.
------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
Tigervnc-devel mailing list
Tigervnc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
