Review: fix JPEG RGBA->RGB conversion

Fix the JPEGOutput's implicit RGBA->RGB conversion.  We were getting all the strides wrong before and botching the output.

Background info:  Most format writers would consider it an error for the caller to request writing more color channels than can be supported by the file format.  But as a special case, we let the JPEG writer silently drop the alpha channel and write as RGB.  The justification is that it's simply too common to deal with RGBA images, very unusual for a format to not support it at all, and too frustrating to users to treat it as an error.

[Chris Foster, 2010-12-06 07:01:31]

Maybe I need to write an image input or output myself to understand what's going
on here...

http://codereview.appspot.com/3446042/diff/1/src/jpeg.imageio/jpegoutput.cpp
File src/jpeg.imageio/jpegoutput.cpp (right):

http://codereview.appspot.com/3446042/diff/1/src/jpeg.imageio/jpegoutput.cpp#...
src/jpeg.imageio/jpegoutput.cpp:263: m_spec.nchannels = save_nchannels;
I'm confused by this!  Can you enlighten me as to how running to_native_scanline
after setting m_spec.nchannels = 3 here results in different strides than
setting m_spec.nchannels = 3 inside open() ?

[larrygritz, 2010-12-06 18:33:59]

On 2010/12/06 07:01:31, Chris Foster wrote:
> src/jpeg.imageio/jpegoutput.cpp:263: m_spec.nchannels = save_nchannels;
> I'm confused by this!  Can you enlighten me as to how running
to_native_scanline
> after setting m_spec.nchannels = 3 here results in different strides than
> setting m_spec.nchannels = 3 inside open() ?

If you set m_spec.nchannels = 3 inside open(), then a call to write_image()
using AutoStride will end up with all wrong strides as it makes the individual
write_scanline calls.

That's what was broken before -- the user has a 4-channel buffer, but we want to
write a 3-channel image; if we doctor the spec to think the input is 3-channel,
that will screw us up in other places.  On the other hand, if we DON'T doctor
the spec, then that low-level call to to_native_scanline will be wrong.  The
ugly compromise is to doctor the spec just in time for to_native_scanline, then
restore it before returning to the caller.  I'm not especially proud of this,
but I haven't thought of anything better that doesn't create a problem
elsewhere.  At least here, the ugliness is confined to the JPEG reader itself.

[Chris Foster, 2010-12-07 02:23:30]

On Tue, Dec 7, 2010 at 4:33 AM,  <larrygritz@gmail.com> wrote:
> If you set m_spec.nchannels = 3 inside open(), then a call to
> write_image() using AutoStride will end up with all wrong strides as it
> makes the individual write_scanline calls.

Aha, that's what I was missing; I was thinking the user would call
write_scanline() directly, forgetting that the spec was also used in
write_image().  It would be nice to tack an extra note about that onto the
"Here's where we do the dirty work" comment.  Other than that, LGTM.

~Chris