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

[Baptiste Coudurier]

Michael Niedermayer wrote:
> 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

Ok, patch attached.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: raw_packet_buffer2.patch
Type: text/x-diff
Size: 2323 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090624/ac554d99/attachment.patch>