[FFmpeg-devel] [PATCH v3 2/2] avformat: add demuxer for argonaut games' ASF format

[Carl Eugen Hoyos]

Am Mi., 22. Jan. 2020 um 00:33 Uhr schrieb Zane van Iperen
<zane at zanevaniperen.com>:
>
> 21/1/20 11:14 pm, Carl Eugen Hoyos пишет:
> >
> > Am Di., 21. Jan. 2020 um 11:09 Uhr schrieb Zane van Iperen
> > <zane at zanevaniperen.com>:
> >
> >> +static int argo_asf_probe(const AVProbeData *p)
> >> +{
> >> +    int score;
> >> +    ArgoASFFileHeader hdr;
> >> +
> >> +    av_assert0(AVPROBE_PADDING_SIZE >= ASF_FILE_HEADER_SIZE);
> >> +
> >> +    argo_asf_parse_file_header(&hdr, p->buf);
> >> +
> >> +    if (hdr.magic != ASF_TAG)
> >> +        return 0;
> >
> >> +    /* If this is huge, then it's very likely not an ASF file. */
> >> +    if (hdr.chunk_offset > INT_MAX)
> >> +        return 1;
> >
> > Am I correct that this tests only one bit?
> > This makes sense for many probe functions that can easily
> > return false positives based on weak heuristics, probe
> > functions typically return high scores for matching first 32
> > bits though.
>
> I can just get rid of that check completely if it's easier.

Please.

> >> +    score = 0;
> >> +    if (argo_asf_is_known_version(&hdr))
> >> +        score += (AVPROBE_SCORE_MAX / 2) + 1;
> >
> > Unusual code style.
> >
> >> +    /* Have only ever seen these with 1 chunk. */
> >> +    if (hdr.num_chunks == 1)
> >> +        score += 10;
> >
> > Returns 0 for argo files with unknown version and more than
> > one chunk and 10 for for unknown version and one chunk.
> >
> > Should be ~25 in both cases, feel free to return a higher
> > score if all tests pass, I just wanted to simplify the probe
> > function as testing 32 bit is what is usually done.
> >
>
> Returning 0 on a unknown version is a bug, I want it to hit the
> avpriv_request_sample() below.
>
> Would something like this (untested) be better?
>
>  >     if (hdr.chunk_offset & 0xFF000000U)
>  >         return 1;
>  >
>  >     score = (AVPROBE_SCORE_MAX / 2) + 1;
>  >
>  >     if (!argo_asf_is_known_version(&hdr))
>  >         score -= 25;
>  >
>  >     /* Have only ever seen these with 1 chunk. */
>  >     if (hdr.num_chunks > 1)
>  >         score -= 25;
>  >
>  >     return score;
>
> That way if it fails one check it'll be 26, or 1 if it fails both.

But it should return ~25 if the first 32bit match (but the version
is wrong) and fail later.

I mostly wanted to argue for a less complicated probe function
for a format that starts with 32 known bits.

Carl Eugen