[FFmpeg-devel] [PATCH] SoX native format muxer and demuxer

[Michael Niedermayer]

On Wed, May 27, 2009 at 09:49:33PM -0400, Daniel Verkamp wrote:
> On Wed, May 27, 2009 at 9:44 PM, Daniel Verkamp <daniel at drv.nu> wrote:
> > On Wed, May 27, 2009 at 8:41 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> uOn Wed, May 27, 2009 at 07:25:13PM -0400, Daniel Verkamp wrote:
> >>> On Wed, May 27, 2009 at 6:29 PM, Daniel Verkamp <daniel at drv.nu> wrote:
> >>> > Hi,
> >>> >
> >>> > Attached is a patch to add a muxer and demuxer for the (very simple)
> >>> > SoX native format.
> >>> >
> >>> > This requires my previous "get/put_{le,be}_{float,double}" patch.
> >>> >
> >>>
> >>> New patch that uses av_int2dbl etc. instead of get_le_double etc.
> >>>
> >>> Please ignore the original patch. :)
> >> [...]
> >>> +#define SOX_TAG_LE MKTAG('.', 'S', 'o', 'X')
> >>> +#define SOX_TAG_BE MKTAG('X', 'o', 'S', '.')
> >>
> >> these seem a little redundant relative to each other
> >>
> >
> > Right, cleaned up.
> >
> >> [...]
> >>> +static int sox_read_header(AVFormatContext *s,
> >>> + ? ? ? ? ? ? ? ? ? ? ? ? ? AVFormatParameters *ap)
> >>> +{
> >>> + ? ?ByteIOContext *pb = s->pb;
> >>> + ? ?unsigned magic, header_size, comment_size, le;
> >>> + ? ?double sample_rate, sample_rate_frac;
> >>> + ? ?AVStream *st;
> >>> +
> >>> + ? ?magic = get_le32(pb);
> >>> + ? ?if (magic == SOX_TAG_LE) {
> >>> + ? ? ? ?le = 1;
> >>> + ? ?} else if (magic == SOX_TAG_BE) {
> >>> + ? ? ? ?le = 0;
> >>> + ? ?} else
> >>> + ? ? ? ?return -1;
> >>
> >> le = (get_le32(pb) == SOX_TAG_LE);
> >>
> >
> > This doesn't check for an invalid tag, but I guess if the user forces
> > the wrong format, he's asking for it...
> >
> >>
> >>> +
> >>> + ? ?st = av_new_stream(s, 0);
> >>> + ? ?if (!st)
> >>> + ? ? ? ?return AVERROR(ENOMEM);
> >>> +
> >>> + ? ?st->codec->codec_type = CODEC_TYPE_AUDIO;
> >>> +
> >>> + ? ?if (le) {
> >>> + ? ? ? ?st->codec->codec_id = CODEC_ID_PCM_S32LE;
> >>> + ? ? ? ?header_size ? ? ? ? = get_le32(pb);
> >>> + ? ? ? ?url_fskip(pb, 8); /* sample count */
> >>> + ? ? ? ?sample_rate ? ? ? ? = av_int2dbl(get_le64(pb));
> >>> + ? ? ? ?st->codec->channels = get_le32(pb);
> >>> + ? ? ? ?comment_size ? ? ? ?= get_le32(pb);
> >>> + ? ?} else {
> >>> + ? ? ? ?st->codec->codec_id = CODEC_ID_PCM_S32BE;
> >>> + ? ? ? ?header_size ? ? ? ? = get_be32(pb);
> >>> + ? ? ? ?url_fskip(pb, 8); /* sample count */
> >>> + ? ? ? ?sample_rate ? ? ? ? = av_int2dbl(get_be64(pb));
> >>> + ? ? ? ?st->codec->channels = get_be32(pb);
> >>> + ? ? ? ?comment_size ? ? ? ?= get_be32(pb);
> >>> + ? ?}
> >>> +
> >>> + ? ?if (sample_rate <= 0 || sample_rate > INT_MAX) {
> >>> + ? ? ? ?av_log(s, AV_LOG_ERROR, "invalid sample rate (%f)\n", sample_rate);
> >>> + ? ? ? ?return -1;
> >>> + ? ?}
> >>> +
> >>> + ? ?sample_rate_frac = sample_rate - floor(sample_rate);
> >>> + ? ?if (sample_rate_frac)
> >>> + ? ? ? ?av_log(s, AV_LOG_WARNING,
> >>> + ? ? ? ? ? ? ? "truncating fractional part of sample rate (%f)\n",
> >>> + ? ? ? ? ? ? ? sample_rate_frac);
> >>> +
> >>> + ? ?if (((header_size + 4) & 7) || header_size < SOX_FIXED_HDR + comment_size
> >>
> >>> + ? ? ? ?|| (st->codec->channels > 65535)) /* Reserve top 16 bits */ {
> >>
> >> superflous ()
> >>
> >
> > Cleaned up (this line was lifted from the SoX code ;).
> >
> >>
> >>> + ? ? ? ?av_log(s, AV_LOG_ERROR, "invalid header\n");
> >>> + ? ? ? ?return -1;
> >>> + ? ?}
> >>> +
> >>> + ? ?if (comment_size) {
> >>> + ? ? ? ?char *comment= av_mallocz(comment_size + FF_INPUT_BUFFER_PADDING_SIZE);
> >>
> >> possible integer overflow
> >>
> >
> > Hopefully fixed - if for some reason there is a file that has such a
> > huge comment_size coded, the new patch should at least skip it
> > properly.
> >
> 
> Actually, with a comment size that large, header size could not be
> properly coded in the bits available, and the file would be invalid
> anyway, so here's a better check.

[...]

> +    le = (get_le32(pb) == SOX_TAG);
> +
> +    st = av_new_stream(s, 0);
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +
> +    st->codec->codec_type = CODEC_TYPE_AUDIO;
> +
> +    if (le) {

is the temporary le even needed ?


[...]
> +    if ((header_size + 4) & 7 || header_size < SOX_FIXED_HDR + comment_size
> +        || st->codec->channels > 65535) /* Reserve top 16 bits */ {
> +        av_log(s, AV_LOG_ERROR, "invalid header\n");
> +        return -1;
> +    }
> +
> +    if (comment_size) {
> +        char *comment = av_mallocz(comment_size + FF_INPUT_BUFFER_PADDING_SIZE);

that still could overflow with a suitable FF_INPUT_BUFFER_PADDING_SIZE
you know what the next line would do in that case?

> +        if (get_buffer(pb, comment, comment_size) != comment_size) {

[...]
> +    for (pad = comment_size - comment_len; pad; pad--)
> +        put_byte(pb, 0);

the pad variable is superflous


[...]
> +static int sox_write_trailer(AVFormatContext *s)
> +{
> +    SoXContext *sox = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    AVCodecContext *enc = s->streams[0]->codec;

> +    int64_t file_size;
> +
> +    if (!url_is_streamed(s->pb)) {
> +        int64_t num_samples;
> +        /* update number of samples */
> +        file_size = url_ftell(pb);
> +        num_samples = (file_size - sox->header_size - 4) >> 2LL;

declaration & init can be merged


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090528/d3272555/attachment.pgp>