[FFmpeg-devel] [PATCH 2/3] Add muxer/demuxer for raw codec2 and .c2 files

[Tomas Härdin]

fre 2018-01-12 klockan 21:21 +0100 skrev Michael Niedermayer:
> On Sat, Dec 23, 2017 at 11:15:35PM +0100, Tomas Härdin wrote:
> > 
> 
> [...]
> > +static int codec2_read_header_common(AVFormatContext *s, AVStream *st)
> > +{
> > +    int mode = avpriv_codec2_mode_from_extradata(st->codecpar->extradata);
> > +
> > +    st->codecpar->codec_type        = AVMEDIA_TYPE_AUDIO;
> > +    st->codecpar->codec_id          = AV_CODEC_ID_CODEC2;
> > +    st->codecpar->sample_rate       = 8000;
> > +    st->codecpar->channels          = 1;
> > +    st->codecpar->format            = AV_SAMPLE_FMT_S16;
> > +    st->codecpar->channel_layout    = AV_CH_LAYOUT_MONO;
> > +    st->codecpar->bit_rate          = avpriv_codec2_mode_bit_rate(s, mode);
> > +    st->codecpar->frame_size        = avpriv_codec2_mode_frame_size(s, mode);
> > +    st->codecpar->block_align       = avpriv_codec2_mode_block_align(s, mode);
> > +
> > +    if (st->codecpar->bit_rate <= 0 ||
> > +        st->codecpar->frame_size <= 0 ||
> > +        st->codecpar->block_align <= 0) {
> > +        return AVERROR(EINVAL);
> > +    }
> 
> This should be AVERROR_INVALIDDATA i think

OK, seem reasonable. It will typically be triggered by invalid mode

> > +
> > +    avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
> > +
> > +    //replicating estimate_timings_from_bit_rate() in utils.c to avoid warnings
> > +    if (s->pb && st->codecpar->bit_rate > 0) {
> > +        int64_t filesize = avio_size(s->pb);
> > +        if (filesize > s->internal->data_offset) {
> > +            filesize -= s->internal->data_offset;
> > +            st->duration = av_rescale(8 * filesize,
> > +                                      st->time_base.den,
> > +                                      st->codecpar->bit_rate * (int64_t) st->time_base.num);
> > +        }
> > +    }
> 
> Is this exact ?
> or is a calculation from frame_size / block_align more accurate ?
> the most accurate one should be used

bit_rate is derived from block_align and frame_size in codec2utils.c so
either would be fine.

It strikes me that a better solution would be to suppress the
"Estimating duration from bitrate, this may be inaccurate" warning for
CBR formats. Removed this hunk for now.

> > +static int codec2_read_header(AVFormatContext *s)
> > +{
> > +    [...]
> > +    avio_read(s->pb, st->codecpar->extradata, AVPRIV_CODEC2_EXTRADATA_SIZE);
> 
> The return codes from avio_read and not checked

Replaced with ffio_read_size() and checking for AVERROR

mån 2018-01-15 klockan 00:32 +0100 skrev Carl Eugen Hoyos:
> > 2017-12-23 23:15 GMT+01:00 Tomas Härdin <tjoppen at acc.umu.se>:
> 
> > +//check for 0xC0DEC2, return non-zero if it doesn't match
> > +static int check_magic(uint8_t *ptr) {
> > +    return memcmp(ptr, avpriv_codec2_magic, 3);
> 
> AV_RB24(), or do I miss something?

Good idea. Replaced some of the related read/write functions with
avio_rb24/wb24 too

> 
> > +static int codec2_probe(AVProbeData *p)
> > +{
> > +    int score;
> > +
> > +    //must be at least 7 bytes and start wih 0xC0DEC2
> > +    if (p->buf_size < AVPRIV_CODEC2_HEADER_SIZE ||
> 
> This check is unneeded.

Good, removed

> > check_magic(p->buf)) {
> 
> +        return 0;
> +    }
> +
> +    //no .c2 files prior to 0.8
> 
> +    if (p->buf[3] == 0 && p->buf[4] < 8) {
> 
> You chose to define the versions, please use the defines here.

Fixed

> +        return 0;
> +    }
> +
> +    //give a poor score if major version doesn't match
> +    //this allows such files to be detected at least, even if we
> can't do much with them
> +    if (p->buf[3] != EXPECTED_CODEC2_MAJOR_VERSION) {
> +        return AVPROBE_SCORE_MAX/10;
> +    }
> 
> That may be ok.

Actually, I feel like being more strict and coordinating future format
changes with the FreeDV folks. Changed this to rejecting major != 0.

> +    //if the minor version is known, no unknown mode is used and no
> flags are set then -> max score,
> +    //else penalize 20% for each byte outside of expectations
> +    //this way if only the first four bytes are OK then we're
> slightly less than AVPROBE_SCORE_MAX/2
> +    //cap score at max-1 unless file extension is .c2
> +    score = AVPROBE_SCORE_MAX;
> 
> +    if (!av_match_ext(p->filename, "c2"))           score -= 1;
> 
> We don't do this for any other demuxer, if this makes sense, it should
> be done in general code, not for a specific demuxer imo.

Removed it, see below.

> +    if (p->buf[4] >  EXPECTED_CODEC2_MINOR_VERSION) score -=
> AVPROBE_SCORE_MAX/5;
> +    if (p->buf[5] >  AVPRIV_CODEC2_MODE_MAX)        score -=
> AVPROBE_SCORE_MAX/5;
> +    if (p->buf[6] != 0)                             score -=
> AVPROBE_SCORE_MAX/5;
> +    return score;
> 
> Imo, this is too complicated:
> If the first 32bit are correct, return MAX/2, for 24bit, return less
> 
> Returning MAX for less than 64bit seems wrong to me.

I think my reasoning was that if everything is within what the FreeDV
tools are currently outputting then a higher score may be OK. On the
other hand I can lobby the FreeDV list for more identifying bits
together with a major version bump in the future, if needed.

Changed to returning MAX/2 since we have C0 DE C2 00. Updated patch
attached.

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-muxer-demuxer-for-raw-codec2-and-.c2-files.patch
Type: text/x-patch
Size: 14194 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180115/915b4f33/attachment.bin>