[FFmpeg-devel] add MJPEG support into RTP output

[Martin Storsjö]

On Fri, 9 Apr 2010, Nash Tsai wrote:

> On Thu, Apr 8, 2010 at 11:35 PM, Martin Storsj? <martin at martin.st> wrote:
> > On Thu, 8 Apr 2010, Nash Tsai wrote:
> >
> >> if (htonl(offset) != offset)
> >> ? ?jpghdr.off = htonl(offset) >> 8; /* we only interested in least
> >> significant 3 octets here */
> >> else
> >> ? ?jpghdr.off = offset;
> >
> > This is just horribly obfuscated. As far as I'm concerned, any patch doing
> > things this way is rejected.
> >
> > Just declare a uint8_t hdr[8] and write each field manually into the
> > correct place of the buffer. For writing the offset you can use e.g.:
> >
> > AV_WB24(&hdr[1], offset);
> 
> Wasn't aware the existence of AV_WB24, thanks, then I would write as
> following version as simply update the packet buffer after jpghdr has
> written into it:
> 
> +        memcpy(q, &jpghdr, sizeof(jpghdr));
> +        AV_WB24(&q[1], jpghdr.off);
> +        memcpy(q + sizeof(jpghdr), buf1, len);
> +        ff_rtp_send_data(s1, q, len + sizeof(jpghdr), (len == size));
> +
> +        buf1 += len;
> +        size -= len;
> +        jpghdr.off += len;
> 
> So I still get to maintain the details of jpghdr with precise struct
> member naming and values.

No, you need to get rid of the jpghdr struct completely. You cannot assume 
that its size and field layout will be consistent across architectures, 
and it is all too easy to make more endianness mistakes like the one with 
the offset. People compiling with other compiler flags may also get a 
struct with a different layout not matching the one in the RFC.

// Martin