[FFmpeg-devel] [PATCH] Add SUP/PGS subtitle demuxer

[wm4]

On Sun, 31 Aug 2014 10:24:13 +0200
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> On Sun, Aug 31, 2014 at 10:15:56AM +0200, Hendrik Leppkes wrote:
> > On Sun, Aug 31, 2014 at 9:41 AM, Reimar Döffinger
> > <Reimar.Doeffinger at gmx.de> wrote:
> > > On 30.08.2014, at 15:38, wm4 <nfxjfg at googlemail.com> wrote:
> > >> +    // The packet-size is stored as part of the packet.
> > >> +    if ((ret = avio_read(s->pb, tmp, 3)) < 0)
> > >> +        return ret;
> > >> +
> > >> +    len = AV_RB16(tmp + 1);
> > >> +
> > >> +    if ((ret = av_new_packet(pkt, len + 3)) < 0)
> > >> +        return ret;
> > >> +
> > >> +    memcpy(pkt->data, tmp, 3);
> > >> +
> > >> +    if ((ret = avio_read(s->pb, pkt->data + 3, len)) < 0) {
> > >> +        av_free_packet(pkt);
> > >> +        return ret;
> > >> +    }
> > >
> > > I think this will not handle short reads correctly, retuning uninitialised data.
> > > My suggestion would be to read the length, then seek back (buffering should ensure this is no issue even if we read from stdin) and then use the functions to read the full packet with all the proper error handling.
> > 
> > 
> > I don't see a problem in the code he proposed. It seems to handle
> > every read with an error check, without relying on potential buffering
> > of data to allow a seek backwards.
> > This is assuming avio_read does return an error if it runs against
> > EOF, which I assume it does? I didn't double check that.
> 
> Why would it? That would make it a huge pain to read formats where
> you don't know ahead of time how long they are (e.g. streaming
> TS files via stdin).
> 
> > What exactly do you think is wrong here?
> 
> The code does not handle 0 < ret < len (I think 0 should not be
> possible), plus it is complex and I am not at all confident it's
> the only case it misses.
> There is a reason we have helper functions.
> If you absolutely do not want to seek back, there is also the
> option of reading a 3-byte packet (but then like now you have
> to add handling getting fewer than that) and then use the function
> to expand the packet to read the rest.
> However there is a good reason why seeking back small amounts is
> _supposed_ to work always, 100% reliable, and it should be fine
> to take advantage of it.

When I looked at avio_read, it seemed like it doesn't allow short
reads, so I simplified the error checks.