Hi,
On Wed, Mar 23, 2011 at 5:03 PM, Kostya <[email protected]> wrote:
> On Tue, Mar 22, 2011 at 06:54:38PM +0100, Diego Biurrun wrote:
>> On Sun, Mar 20, 2011 at 11:02:23AM +0100, Kostya wrote:
>> > [...]
>>
>> The usual nits..
>
> reordered, added whitespaces, broke some long lines (even if I don't know what
> to do with all that karma), fixed license and added missing long name
[..]
Demuxer review:
> +static av_cold int read_vinfo0(AVIOContext* pb, sanm_vinfo* pvinfo)
[..]
> + size = avio_rb32(pb);
> + endpos = avio_tell(pb) + size;
Shouldn't size be checked for validity? E.g. if size < 256*3, it is
certainly invalid, or if size > 256*4, it is likely invalid, right? At
least a warning would be useful then.
Also, width/height are initialized to zero here, are they filled in
later? Is that checked?
> +static av_cold int read_vinfo1(AVIOContext* pb, sanm_vinfo* pvinfo)
[..]
> + uint32_t size, sig = avio_rb32(pb);
[..]
> + size = avio_rb32(pb);
> + endpos = avio_tell(pb) + size;
[..]
> + avio_seek(pb, endpos, SEEK_SET);
read_vinfo0() also appears to do this, can it be done in the calling
function? Would save some code. I see it repeated below also. Also,
here, size isn't checked for validity. size=0 is likely a broken file.
> +static av_cold int read_ainfo1(AVIOContext* pb, sanm_ainfo* painfo)
[..]
> + done = 0;
> + while (!done) {
> + sig = avio_rb32(pb);
> + size = avio_rb32(pb);
> +
> + switch (sig) {
> + case MKBETAG('W', 'a', 'v', 'e'):
> + done = 1;
> + painfo->freq = avio_rl32(pb);
> + painfo->nchannels = avio_rl32(pb);
> + break;
> +
> + case MKBETAG('B', 'l', '1', '6'):
> + avio_seek(pb, size, SEEK_CUR);
> + break;
> +
> + default:
> + done = 1;
> + break;
> + }
> + }
This may be unlikely, but since this appears RIFF-based, what if an
unknown chunk precedes the Wave chunk? Parsing would fail. Should the
condition be
while (url_ftell(pb) + 8 < end_pos), and then the default skip the
chunk (like Bl16) also?
> +static int sanm_read_header(AVFormatContext* ctx, AVFormatParameters* ap)
[..]
> + switch(movie_sig){
> + case MKBETAG('A', 'N', 'I', 'M'):
> + pctx->version = 0;
> + break;
> + case MKBETAG('S', 'A', 'N', 'M'):
> + pctx->version = 1;
> + break;
> + default:
> + av_log(ctx, AV_LOG_ERROR, "Wrong magic\n");
> + return AVERROR_INVALIDDATA;
> + }
> +
> + avio_rb32(pb); // skip movie size
> +
> + if (!pctx->version && read_vinfo0(pb, &pctx->vinfo)) {
> + av_log(ctx, AV_LOG_ERROR, "Wrong video stream header\n");
> + return AVERROR_INVALIDDATA;
> + }
> + if (pctx->version && read_vinfo1(pb, &pctx->vinfo)) {
> + av_log(ctx, AV_LOG_ERROR, "Wrong video stream header\n");
> + return AVERROR_INVALIDDATA;
> + }
The switch(movie_sig) can be moved below the avio_rb32(), and then the
read_vinfo[01]() can be done inside the switch also.
> + pvstream->codec->codec_tag = MKBETAG('S', 'A', 'N', 'M');
Is that necessary?
> + if (!pctx->version) {
> + int i;
> + pvstream->codec->extradata = av_malloc(1024+2);
> + pvstream->codec->extradata_size = 1024 + 2;
> + AV_WL16(pvstream->codec->extradata, pvinfo->subversion);
> + for(i = 0; i < 256; i++)
> + AV_WL32(pvstream->codec->extradata + 2 + i*4,
> pvinfo->palette[i]);
> + }
Can you directly write the palette into extradata instead of using
pvinfo->palette[] as an intermediate buffer?
> + if (pctx->version && read_ainfo1(pb, &pctx->ainfo)) {
> + av_log(ctx, AV_LOG_ERROR, "Invalid audio information\n");
> + return AVERROR_INVALIDDATA;
> + }
This can also be done inside the switch, right?
Is the order of chunks completele fixed, or can chunks be ordered
differently, or can unknown chunks be inserted in the middle? Since
it's riff-based, a slightly more generic parser might be able to
handle more files from different games using the same codec/format.
Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
