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

[Reimar Döffinger]

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.