[Spice-devel] [PATCH spice-gtk v2] channel-display-mjpeg: Fix encoding for big endian machines

[Victor Toso]

Hi,

On Thu, Nov 15, 2018 at 03:22:04PM -0500, Frediano Ziglio wrote:
> > 
> > On Fri, Nov 02, 2018 at 09:38:12AM +0000, Frediano Ziglio wrote:
> > > Make sure components are ordered in the same way in memory.
> > > This was tested with a virtual MIPS machine.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  src/channel-display-mjpeg.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > Changes since v1:
> > > - fix typo in commit message title
> > > 
> > > diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> > > index 83cd391b..94e56205 100644
> > > --- a/src/channel-display-mjpeg.c
> > > +++ b/src/channel-display-mjpeg.c
> > > @@ -108,7 +108,11 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> > > video_decoder)
> > >  
> > >  #ifdef JCS_EXTENSIONS
> > >      // requires jpeg-turbo
> > > +#  if SPICE_ENDIAN == SPICE_ENDIAN_LITTLE
> > >      decoder->mjpeg_cinfo.out_color_space = JCS_EXT_BGRX;
> > > +#  else
> > > +    decoder->mjpeg_cinfo.out_color_space = JCS_EXT_XRGB;
> > > +#  endif
> >     ^^
> > Why the spaces
> > 
> 
> indentation.

I got that. I just don't see elsewhere in the code. I don't
really mind.

> Maybe something like:
> 
>     if (SPICE_ENDIAN == SPICE_ENDIAN_LITTLE) {
>         decoder->mjpeg_cinfo.out_color_space = JCS_EXT_BGRX;
>     } else {
>         decoder->mjpeg_cinfo.out_color_space = JCS_EXT_XRGB;
>     }
> 
> would be better. It looks like less efficient but if the compiler
> cannot turn this to just an assignment I would consider more a bug in
> the compiler. Also this form has the advantage to compile all code
> (big endian machines are neither common nor much tested).

I'd say to stick with v1. Don't recall the last time I saw
someone discussing big endian clients.

Cheers,

Acked-by: Victor Toso <victortoso at redhat.com>

> > >  #else
> > >  #warning "You should consider building with libjpeg-turbo"
> > >      decoder->mjpeg_cinfo.out_color_space = JCS_RGB;
> 
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20181116/e34454c9/attachment.sig>