[FFmpeg-devel] add MJPEG support into RTP output

[Martin Storsjö]

Hi,

On Thu, 8 Apr 2010, shawn shawn wrote:

>     I've added MJPEG support into RTP output, please take look.


> +/* Motion-JPEG RTP Payload Header */
> +struct JPEGHdr {
> +   uint32_t tspec:8; /* type-specific field */
> +   uint32_t off:24; /* fragment byte offset */
> +   uint8_t type; /* id of jpeg decoder params */
> +   uint8_t q; /* quantization factor (or table id) */
> +   uint8_t width; /* frame width in 8 pixel blocks */
> +   uint8_t height; /* frame height in 8 pixel blocks */
> +};

Writing a struct out to the network like this requires us to assume quite 
a bit about the struct packing - I'm not sure we can assume that. 
Especially regarding the layout of the bitfields - is that endian safe?

> +/* incremen sequence number */

Missing a 't' in the comment

>  }
>  
> +
>  /* send an integer number of samples and compute time stamp and fill

Stray extra line, not related to the actual changes

>     the rtp send buffer before sending. */
>  static void rtp_send_samples(AVFormatContext *s1,
> @@ -282,7 +311,6 @@
>          s->buf_ptr += size;
>      }
>  }
> -
>  static void rtp_send_raw(AVFormatContext *s1,

Dito

> @@ -385,6 +452,11 @@
>      case CODEC_ID_H263P:
>          ff_rtp_send_h263(s1, pkt->data, size);
>          break;
> +    case CODEC_ID_MJPEG:
> +       {
> +         ff_rtp_send_mjpeg(s1, pkt->data, size, st->codec->width, st->codec->height);
> +       }
> +       break;

Weird indentation. No braces are needed for this.

> +        case CODEC_ID_MJPEG:
> +            av_strlcatf(buff, size, "a=rtpmap:%d MJPEG/30000\r\n",
> +                                     payload_type);
> +            break;

Are you sure of this? All other video codecs I've seen in RTP use 90000 as 
a time base.

What players have you tested this with, and in which RFC is it 
standardized? (Make sure to add the RFC reference as a code comment.)


All current packetizers buffer the data they're going to send within 
RTPMuxContext->buf, but splitting ff_rtp_send_data this way does allow us 
to avoid one layer of buffering, if each individual packetizer wants to. 
You need to hear from Luca A if he's ok with this approach. If he is, 
splitting ff_rtp_send_data should be a separate patch, and the split out 
functions should be added to rtpenc.h, so that other packetizers can use 
them.

Also, should this be in a separate file, rtpenc_mjpeg.c?

// Martin