[FFmpeg-devel] [PATCH][RFC] QCP demuxer

Michael Niedermayer michaelni
Sun May 17 00:05:19 CEST 2009


On Sat, May 16, 2009 at 12:01:09PM -0700, Kenan Gillet wrote:
> On Fri, May 15, 2009 at 6:46 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Fri, May 15, 2009 at 06:43:53PM -0700, Kenan Gillet wrote:
> >>
> >> On May 15, 2009, at 6:19 PM, Michael Niedermayer wrote:
> >>
> >>> On Fri, May 15, 2009 at 06:01:27PM -0700, Kenan Gillet wrote:
> >>>>
> >>>> On May 15, 2009, at 4:56 PM, Michael Niedermayer wrote:
> >>>>
> >>>>> On Fri, May 15, 2009 at 03:36:36PM -0700, Kenan Gillet wrote:
> >>>>>> On Fri, May 15, 2009 at 2:16 PM, Michael Niedermayer <michaelni at gmx.at>
> >>>>>> wrote:
> >>>>>>> On Fri, May 15, 2009 at 10:46:12PM +0200, Reimar D?ffinger wrote:
> >>>>>>>> On Fri, May 15, 2009 at 11:21:21AM -0700, Kenan Gillet wrote:
> >>>>>>>>> Any other solution i missed?
> >>>>>>>>
> >>>>>>>> e.g. 8-entry LUT and & 7
> >>>>>>>
> >>>>>>> this does not appear particularly robust in light of a damaged file
> >>>>>>>
> >>>>>>> [...]
> >>>>>>
> >>>>>> A file with its rate-map-table corrupted but using a fixed rate
> >>>>>> would still be ?playable, as one with a corrupted but unused entry.
> >>>>>> So being permissive would be nice, plus, some invalid data would
> >>>>>> be detected later if a corrupted entry is used.
> >>>>>>
> >>>>>
> >>>>>> FIY, I have a sample, a trashed file, with a corrupted rate-map-table,
> >>>>>> which still played correctly with the solution attached which make it
> >>>>>> round 3.
> >>>>>
> >>>>> the probability of a 8 entry table to be corrupted while the rest of the
> >>>>> file is not is rather small, that is compared to "implementation error"
> >>>>> in that sense, are you sure your implementation is correct?
> >>>>
> >>>> I check the code against the RFC, and i believe it is correct.
> >>>> The rate-map-table is only used if the ?var-rate-flag is non zero.
> >>>> If zero, the file then contains packets of fixed length and the table
> >>>> is not used.
> >>>>
> >>>> After further investigation, the trashed file was generating an empty
> >>>> wav file, so the changes were not making any difference in the output.
> >>>>
> >>>>
> >>>
> >>>>> and what makes you think it is corrupted?
> >>>>
> >>>> I used the trasher tools.
> >>>
> >>> good point but then id assume that not just that table is off
> >>>
> >>>
> >>>> BTW, is there any rule of thumb for a good count/maxburst/filesize ratio
> >>>> when using the trasher tool?
> >>>>
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>> + ? ? ? ? ? ?int ret, mode = get_byte(pb);
> >>>>>> + ? ? ? ? ? ?int pkt_size ?= c->fixed_packet_size ?
> >>>>>> c->fixed_packet_size
> >>>>>> - 1
> >>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? :
> >>>>>> c->rates_per_mode[mode];
> >>>>>
> >>>>> this still has the same problem of crashing almost instantly
> >>>>> and being very fragile even if not crashing
> >>>>
> >>>> what about:
> >>>>
> >>>> int pkt_size, ret, mode = get_byte(pb);
> >>>>
> >>>> if (c->fixed_packet_size) {
> >>>> ? ?pkt_size = c->fixed_packet_size - 1;
> >>>> } else if (mode <= QCP_MAX_MODE) {
> >>>> ? ?pkt_size = c->rates_per_mode[mode];
> >>>> } else
> >>>> ? ?pkt_size = -1
> >>>
> >>> thats the first part, the next is that you should try to recover
> >>> from bad data instead of failing
> >>
> >> would trying to seek to the next packet to see if it starts with a valid
> >> byte_mode
> >> be ok? all rates in the rate-map table could be tried?
> >>
> >> any other idea or suggestion on where to look at?
> >
> > the simplest: retry with the next byte
> >
> > [...]
> >
> 
> like in patch attached ?

[...]
> +static int qcp_read_header(AVFormatContext *s, AVFormatParameters *ap)
> +{
> +    ByteIOContext *pb = s->pb;
> +    QCPContext    *c  = s->priv_data;
> +    AVStream      *st = av_new_stream(s, 0);
> +    uint8_t       buf[16];
> +    int           i, nb_rates;
> +
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +
> +    get_be32(pb);                    // "RIFF"
> +    s->file_size = get_le32(pb) + 8;
> +    url_fskip(pb, 8 + 4 + 1 + 1);    // "QLCMfmt " + chunk-size + major-version + minor-version
> +
> +    st->codec->codec_type = CODEC_TYPE_AUDIO;
> +    st->codec->channels   = 1;
> +    get_buffer(pb, buf, 16);
> +    if (is_qcelp_13k_guid(buf)) {
> +        st->codec->codec_id = CODEC_ID_QCELP;
> +    } else if (!memcmp(buf, guid_evrc, 16)) {
> +        av_log(s, AV_LOG_ERROR, "EVRC codec is not supported.\n");
> +        return AVERROR_PATCHWELCOME;
> +    } else if (!memcmp(buf, guid_smv, 16)) {
> +        av_log(s, AV_LOG_ERROR, "SMV codec is not supported.\n");
> +        return AVERROR_PATCHWELCOME;
> +    } else {
> +        av_log(s, AV_LOG_ERROR, "Unknown codec GUID.\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +    url_fskip(pb, 2 + 80); // codec-version + codec-name
> +    st->codec->bit_rate = get_le16(pb);
> +

> +    c->fixed_packet_size =
> +    s->packet_size       = get_le16(pb);

redundant?


[...]
> +static int qcp_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    ByteIOContext *pb = s->pb;
> +    QCPContext    *c  = s->priv_data;
> +    unsigned int  chunk_size, tag;
> +
> +    while(!url_feof(pb)) {
> +        if (c->data_size) {
> +            int pkt_size, ret, mode = get_byte(pb);
> +
> +            if (c->fixed_packet_size) {
> +                pkt_size = c->fixed_packet_size - 1;
> +            } else {
> +                int retry;
> +

> +                for (retry = 256; retry > 0 && !url_feof(pb) && mode > QCP_MAX_MODE; retry--) {
> +                    av_log(s, AV_LOG_WARNING, "wrong byte mode=%d@%llx\n", mode, url_ftell(pb) - 1);
> +                    c->data_size--;
> +                    mode = get_byte(pb);
> +                }

this doesnt even cover a single damaged sector of a disk
besides it spams the user to death

the normal way to do this is a
if(mode > QCP_MAX_MODE)
    continue;


> +                if (!retry || url_feof(pb))
> +                    return AVERROR_INVALIDDATA;
> +                pkt_size = c->rates_per_mode[mode];
> +            }
> +
> +            if (c->data_size <= pkt_size) {
> +                av_log(s, AV_LOG_WARNING, "Data chunk is too small.\n");
> +                pkt_size = c->data_size - 1;
> +            }
> +
> +            if ((ret = av_get_packet(pb, pkt, pkt_size)) >= 0) {

pkt_size can be -1 this is ignored


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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20090517/18ef5168/attachment.pgp>



More information about the ffmpeg-devel mailing list