[FFmpeg-soc] G723.1 Frame Parser

[Martin Storsjö]

Hi,

Some assorted comments below:

On Mon, 5 Apr 2010, Mohamed Naufal wrote:

> +/*
> + * G723.1 frame types
> + */
> +typedef enum {
> +    Frame6k3,           ///< 6.3 kbps frame rate
> +    Frame5k3,           ///< 5.3 kbps frame rate
> +    FrameSID,           ///< silence insertion descriptor frame
> +    FrameUntransmitted
> +} FrameType;

Since these should correspond to actual bitstream, I think it would be 
more readable if at least the first one was explicitly specified as = 0, 
so that the reader knows that it's value must match something defined in 
the bitstream, i.e., it isn't a totally internal enum that can be 
redefined as needed. But the first one is implicitly zero according to C99 
anyway, so it isn't strictly required.

>  OBJS-$(CONFIG_RTSP_DEMUXER)              += rtsp.o httpauth.o
>  OBJS-$(CONFIG_RTSP_MUXER)                += rtsp.o rtspenc.o httpauth.o
> -OBJS-$(CONFIG_SDP_DEMUXER)               += rtsp.o        \
> -                                            rdt.o         \
> -                                            rtp.o         \
> -                                            rtpdec.o      \
> -                                            rtpdec_amr.o  \
> -                                            rtpdec_asf.o  \
> -                                            rtpdec_h263.o \
> -                                            rtpdec_h264.o \
> +OBJS-$(CONFIG_SDP_DEMUXER)               += rtsp.o          \
> +                                            rdt.o           \
> +                                            rtp.o           \
> +                                            rtpdec.o        \
> +                                            rtpdec_amr.o    \
> +                                            rtpdec_asf.o    \
> +                                            rtpdec_g723_1.o \
> +                                            rtpdec_h263.o   \
> +                                            rtpdec_h264.o   \
>                                              rtpdec_theora.o \
>                                              rtpdec_vorbis.o

Cosmetic changes on the other lines, keep your change to the actually 
added lines, then the backslashes can be fixed separately later. Also, 
this file has changed in SVN a few days ago, so you'd better rebase your 
patch on top of that.

> diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
> index e329ad2..66a0f9f 100644
> --- a/libavformat/rtpdec.c
> +++ b/libavformat/rtpdec.c
> @@ -32,6 +32,7 @@
>  #include "rtpdec.h"
>  #include "rtpdec_amr.h"
>  #include "rtpdec_asf.h"
> +#include "rtpdec_g723_1.c"
>  #include "rtpdec_h263.h"

You should be including the .h, not the .c

> +/*
> + * RTP G.723.1 Depacketizer, RFC 3551, 3555
> + */
> +#include "avformat.h"
> +#include "rtpdec_g723_1.h"
> +#include "libavutil/avstring.h"

What do you use avstring.h for here?

> +static const uint8_t frame_sizes[3] = {24, 20, 4};
> +
> +static int g723_1_handle_packet(AVFormatContext *ctx,
> +                                PayloadContext *data,
> +                                AVStream *st,
> +                                AVPacket *pkt,
> +                                uint32_t *timestamp,
> +                                const uint8_t *buf,
> +                                int len, int flags)
> +{
> +    int frames;
> +    int pkt_size = len;
> +
> +    /* The frame size and codec type is determined from the HDR bits
> +     * (buf[frames] & 3) in the first octet of a frame.
> +     */
> +    for (frames = 0; frames < len && (buf[frames] & 3) != 3;
> +         frames += frame_sizes[buf[frames] & 3]);
> +
> +    if (frames > len) {
> +        av_log(ctx, AV_LOG_WARNING, "Too little data in the RTP packet\n");
> +    } else if (frames < len) {
> +        /* This block is executed when the HDR bits are 0b11.
> +         * But this is reserved for future use.
> +         */
> +        av_log(ctx, AV_LOG_WARNING, "Found an invalid frame!\n");
> +        pkt_size = frames;
> +    }
> +
> +    if (av_new_packet(pkt, pkt_size) < 0) {
> +        av_log(ctx, AV_LOG_ERROR, "Out of memory\n");
> +        return AVERROR_NOMEM;
> +    }
> +
> +    pkt->stream_index = st->index;
> +    memcpy(pkt->data, buf, pkt_size);
> +    return 0;
> +}

When you don't do any splitting of frames in the depacketizer, you're 
basically returning the whole packet payload unmodified, right? So this is 
basically only performing some sanity checks on the data, which will also 
be done implicitly by either a parser or ffplay/ffmpeg decoding from the 
same AVPacket multiple times until all data has been consumed. So I'm not 
sure if this really is necessary here, although there's no harm in 
keeping it either.

// Martin