On Sun, 31 Aug 2014 10:24:13 +0200
Reimar Döffinger <reimar.doeffin...@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.doeffin...@gmx.de> wrote:
> > > On 30.08.2014, at 15:38, wm4 <nfx...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
