[FFmpeg-devel] [PATCH] CD+G Demuxer & Decoder

[Michael Niedermayer]

On Sun, Dec 06, 2009 at 10:50:10PM -0800, Michael Tison wrote:
> Hi,
> 
> Attached is a revised patch.
> 
> On Thu, Dec 3, 2009 at 7:29 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Wed, Dec 02, 2009 at 12:44:21AM -0800, Michael Tison wrote:
> > [...]
> >> > [...]
> >> >> +static int read_packet(AVFormatContext *s, AVPacket *pkt)
> >> >> +{
> >> >> + ? ?int ret;
> >> >> + ? ?ByteIOContext *pb = s->pb;
> >> >> +
> >> >
> >> >> + ? ?ret = av_get_packet(pb, pkt, 24);
> >> >> + ? ?if (pb->eof_reached == 1)
> >> >> + ? ? ? return -1;
> >> >
> >> > possible memleak, and wrong return code
> >>
> >> Corrected I hope:
> >> static int read_packet(AVFormatContext *s, AVPacket *pkt)
> >> {
> >> ? ? int ret;
> >> ? ? ByteIOContext *pb = s->pb;
> >>
> >> ? ? ret = av_get_packet(pb, pkt, 24);
> >> ? ? if (ret != 24)
> >> ? ? ? return AVERROR(EIO);
> >>
> >> ? ? pkt->stream_index = 0;
> >> ? ? return 0;
> >> }
> >> I just took this return value from iss.c because I wasn't entirely
> >> sure what to return.
> >
> > it still looks like a memleak, av_get_packet() succeeds and allocates
> > memory and you turn it into a EIO
> 
> Closer to right?
> static int read_packet(AVFormatContext *s, AVPacket *pkt)
> {
>     int ret;
>     ByteIOContext *pb = s->pb;
> 
>     ret = av_get_packet(pb, pkt, CDG_PACKET_SIZE);
> 
>     if (ret != CDG_PACKET_SIZE) {
> 	av_free_packet(pkt);
> 	return ret < 0 ? ret : AVERROR(EIO);
>     }
> 
>     pkt->stream_index = 0;
>     return 0;
> }
> 

> I've noticed in a few formats they call av_free_packet() after a
> failed av_get_packet()

which do this?
If av_get_packet() failed then there is no packet to free.
but not returning the size you want is not a failure from av_get_packets
point of view.

the correct thing to do is

ret = av_get_packet(pb, pkt, CDG_PACKET_SIZE);
return ret;

or if you really want to discard the last packet of a truncated file

ret = av_get_packet(pb, pkt, CDG_PACKET_SIZE);
if(ret >= 0 && ret < CDG_PACKET_SIZE){
    av_free_packet(pkt);
    ret= AVERROR(EIO);
}
return ret;

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

No great genius has ever existed without some touch of madness. -- 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/20091207/f75d1ffb/attachment.pgp>