[FFmpeg-devel] [PATCH] Add libmodplug support.

[Clément Bœsch]

On Wed, Oct 05, 2011 at 07:13:18PM +0200, Reimar Döffinger wrote:
> On Tue, Oct 04, 2011 at 09:11:36PM +0200, Clément Bœsch wrote:
> > +    int sz = avio_read(pb, mikmod->buf, sizeof mikmod->buf);
> 
> In AVInputFormat you use sizeof with (), consistent would be good IMO.
> Also, any chance of getting a probe function?
> 

Fixed.

> > +static int mikmod_read_packet(AVFormatContext *s, AVPacket *pkt)
> > +{
> > +    int ret, n;
> > +    MikModContext *mikmod = s->priv_data;
> > +    uint8_t buf[512];
> > +
> > +    n = ModPlug_Read(mikmod->f, buf, sizeof buf);
> > +    if (n <= 0)
> > +        return AVERROR(EIO);
> > +
> > +    ret = av_new_packet(pkt, n);
> > +    if (ret)
> > +        return ret;
> 
> Only < 0 is really supposed to indicate an error - and even
> if not you should not return > 0 like this here.
> 

Fixed.

> > +    pkt->pts  = pkt->dts = AV_NOPTS_VALUE;
> > +    pkt->size = n;
> 
> I don't think setting the size is necessary if it's the
> same value as in av_new_packet.
> 

size is now used with ModPlug_Read() directly, but pkt pts/dts weren't
necessary so removed.

> > +    memcpy(pkt->data, buf, n);
> 
> And what don't you just do ModPlug_Read directly into
> pkt->data?
> 

Removed, pkt->data is now directly loaded.

> > +static int mikmod_read_seek(AVFormatContext *s, int stream_idx, int64_t ts, int flags)
> > +{
> > +    const MikModContext *mikmod = s->priv_data;
> > +    ModPlug_Seek(mikmod->f, (int)ts);
> 
> What is the point of the cast? And does ModPlug_Seek really take
> the position in samples?

Isn't av_set_pts_info(st, 64, 1, 1000) meaning that ts are expressed in
ms? The cast is just because ModPlug_Seek() needs a int and ts is in
int64_t, but I guess I could remove it. Though, I think it's better to
show the mismatch.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111005/8f387311/attachment.asc>