[FFmpeg-devel] [PATCH] probe max read size

[Michael Niedermayer]

On Wed, Jun 24, 2009 at 12:47:14PM -0700, Baptiste Coudurier wrote:
> Hi Michael,
> 
> Michael Niedermayer wrote:
> > On Wed, Jun 03, 2009 at 03:29:48PM -0700, Baptiste Coudurier wrote:
> >> Michael Niedermayer wrote:
> >>> On Mon, Jun 01, 2009 at 09:50:47PM -0700, Baptiste Coudurier wrote:
> >>>> Michael Niedermayer wrote:
> >>>>> On Mon, Jun 01, 2009 at 03:37:55PM -0700, Baptiste Coudurier wrote:
> >>>>>> Michael Niedermayer wrote:
> >>>>>>> On Mon, Jun 01, 2009 at 12:36:54PM -0700, Baptiste Coudurier wrote:
> >>>>>>>> Michael Niedermayer wrote:
> >>>>>>>>> On Sun, May 31, 2009 at 02:53:34AM -0700, Baptiste Coudurier wrote:
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> After some tests, it seems more reasonable to stop probing after some
> >>>>>>>>>> max size to avoid consuming to much memory.
> >>>>>>>>> Maybe we then should replace probe_packets by a size based thing,
> >>>>>>>>> having 2 is uglier than one if one is sufficient
> >>>>>>>> Sure. Problem is per stream counting is not reasonable since streams
> >>>>>>>> might not have many packets present this will cause much buffering.
> >>>>>>> but without per stream counting streams starting once the threashold is
> >>>>>>> reached should fail to be probed or some streams would only be returned
> >>>>>>> at eof ...
> >>>>>> I think that, in memory constrained systems, user will want to limit:
> >>>>>> - raw_packet_buffer size
> >>>>>> - probing data size (which is btw still a problem, pd->buf must be
> >>>>>> released at some point if codec is not probed).
> >>>>>>
> >>>>>> To achieve this, I think both sizes must be globally bounded.
> >>>>>>
> >>>>>> Maybe by s->probesize or another field, but ideally it must be user
> >>>>>> configurable.
> >>>>> Either way the value that counts the amount remaining must be in a context
> >>>>> not on the stack of av_read_*
> >>>>>
> >>>>> also if it is in the context it can be set by the user
> >>>> Yes, definitely.
> >>>>
> >>>>>>> If we want a packet limit of PL and a byte limit of BL then
> >>>>>>>
> >>>>>>> init(){
> >>>>>>>     counter= BL
> >>>>>>> }
> >>>>>>> ...
> >>>>>>> counter -= packet.size + BL/PL;
> >>>>>>> if(counter > 0)
> >>>>>>>
> >>>>>>> could be used, per stream, iam not sure if this is a good idea or if
> >>>>>>> we rather should keep 2 counter
> >>>>>>>
> >>>>>>> Geometrically, this limit check has an area of a triangle that is within the
> >>>>>>> limit while the normal 2 variable check uses a rectangle, i feel that a triangle
> >>>>>>> is supperior ;)
> >>>>>> Sure, is packet limit still wanted if we have global byte limit though ?
> >>>>> assuming you have a 1mb size limit and a stream containing gsm
> >>>>> it wont ever be probebed successfully because we have no gsm probe
> >>>>> it has 13kbit/sec which means that no packet will be returned to the
> >>>>> user app for ~10 minutes when then finally all the packets will be
> >>>>> returned for these 10min
> >>>>>
> >>>> Patch attached. I believe using probesize is a good opportunity and I
> >>>> plan to replace MAX_READ_SIZE by probesize too, changing probesize
> >>>> default to 5000000.
> >>>>
> >>>> I find the FFMAX a bit ugly but I feel it would be safer.
> >>> [...]
> >>>> @@ -532,8 +534,13 @@ int av_read_packet(AVFormatContext *s, AVPacket *p
> >>>>          if (pktl) {
> >>>>              *pkt = pktl->pkt;
> >>>>              if(s->streams[pkt->stream_index]->codec->codec_id != CODEC_ID_PROBE ||
> >>>> -               !s->streams[pkt->stream_index]->probe_packets){
> >>>> +               !s->streams[pkt->stream_index]->probe_packets ||
> >>>> +               s->raw_packet_buffer_remaining_size <= 0){
> >>>> +                AVProbeData *pd = &st->probe_data;
> >>>> +                av_freep(&pd->buf);
> >>>> +                pd->buf_size = 0;
> >>>>                  s->raw_packet_buffer = pktl->next;
> >>>> +                s->raw_packet_buffer_remaining_size += pkt->size;
> >>>>                  av_free(pktl);
> >>>>                  return 0;
> >>>>              }
> >>>> @@ -567,6 +574,8 @@ int av_read_packet(AVFormatContext *s, AVPacket *p
> >>>>              return ret;
> >>>>  
> >>>>          add_to_pktbuf(&s->raw_packet_buffer, pkt, &s->raw_packet_buffer_end);
> >>>> +        s->raw_packet_buffer_remaining_size =
> >>>> +            FFMAX(0, s->raw_packet_buffer_remaining_size - pkt->size);
> >>>>  
> >>>>          if(st->codec->codec_id == CODEC_ID_PROBE){
> >>>>              AVProbeData *pd = &st->probe_data;
> >>> is the combination of FFMAX here and the += pkt->size; above not able to
> >>> change raw_packet_buffer_remaining_size to a value that is beyond the
> >>> initial limit ?
> >> That's possible indeed, I was wondering is this would be safe. If you
> >> think it's safer to limit it as well, here is an updated patch.
> > 
> > i think it should be possible to remove both FFMIN&MAX
> > we just have to check the remaining size against the packet size instead
> > of against 0 then we can always saftely subtract it
> 
> I'm not sure to understand what you mean here.
> Can you please clarify by code ?
> 
> Do you mean
> if (s->raw_packet_buffer_remaining_size >= pkt->size)
>     remaining_size -= pkt->size;
> 
> ?
> 
> How would you change the check above ?

what i meant is to replace
if(raw_packet_buffer_remaining_size <= 0)
    end
by
if(raw_packet_buffer_remaining_size < next_packet)
    end

that way the next packet will always fit in raw_packet_buffer_remaining_size
and can be subtracted without MIN/MAX

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

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- 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/20090624/ed3afe36/attachment.pgp>