diff mbox

[FFmpeg-devel] avformat: remove request_probe assert from ff_read_packet

Message ID 08deadd4-2dc6-27c3-d6f5-af6a85ca622c@googlemail.com
State Superseded
Headers

Commit Message

Andreas Cadhalpun Oct. 18, 2016, 8:31 p.m. UTC 
Nothing guarantees to set request_probe to -1, so this assert can be
triggered, e.g. if st->probe_packets is 0.

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---

I think the reason why this assert isn't triggered way more often is
that the probing code usually works quite well.

---
 libavformat/utils.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Michael Niedermayer Oct. 18, 2016, 8:56 p.m. UTC | #1 
On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
> Nothing guarantees to set request_probe to -1, so this assert can be
> triggered, e.g. if st->probe_packets is 0.

probe_codec() called with NULL should cause
st->probe_packets = 0
st->request_probe = -1;

[...]
Andreas Cadhalpun Oct. 18, 2016, 9:26 p.m. UTC | #2 
On 18.10.2016 22:56, Michael Niedermayer wrote:
> On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
>> Nothing guarantees to set request_probe to -1, so this assert can be
>> triggered, e.g. if st->probe_packets is 0.
> 
> probe_codec() called with NULL should cause
> st->probe_packets = 0
> st->request_probe = -1;

Yes, but request_probe can be change to a different value later on,
e.g. in ff_parse_mpeg2_descriptor:

int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
{
[...]
            if (s->internal->raw_packet_buffer_remaining_size <= 0)
                if ((err = probe_codec(s, st, NULL)) < 0) // probe_packets = 0, request_probe = -1
                    return err;
[...]
        ret = s->iformat->read_packet(s, pkt);
~~~
ff_parse_mpeg2_descriptor([...])
{
[...]
    switch (desc_tag) {
[...]
    case 0x05: /* registration descriptor */
[...]
                st->request_probe = 50;
[...]
}
~~~
[...]
                if (st->probe_packets) // still 0
                    if ((err = probe_codec(s, st, NULL)) < 0)
                        return err;
                av_assert0(st->request_probe <= 0); // now 50
SIGABRT

Best regards,
Andreas
Hendrik Leppkes Oct. 18, 2016, 9:46 p.m. UTC | #3 
On Tue, Oct 18, 2016 at 11:26 PM, Andreas Cadhalpun
<andreas.cadhalpun@googlemail.com> wrote:
> On 18.10.2016 22:56, Michael Niedermayer wrote:
>> On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
>>> Nothing guarantees to set request_probe to -1, so this assert can be
>>> triggered, e.g. if st->probe_packets is 0.
>>
>> probe_codec() called with NULL should cause
>> st->probe_packets = 0
>> st->request_probe = -1;
>
> Yes, but request_probe can be change to a different value later on,
> e.g. in ff_parse_mpeg2_descriptor:
>
> int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
> {
> [...]
>             if (s->internal->raw_packet_buffer_remaining_size <= 0)
>                 if ((err = probe_codec(s, st, NULL)) < 0) // probe_packets = 0, request_probe = -1
>                     return err;
> [...]
>         ret = s->iformat->read_packet(s, pkt);
> ~~~
> ff_parse_mpeg2_descriptor([...])
> {
> [...]
>     switch (desc_tag) {
> [...]
>     case 0x05: /* registration descriptor */
> [...]
>                 st->request_probe = 50;
> [...]
> }
> ~~~
> [...]
>                 if (st->probe_packets) // still 0
>                     if ((err = probe_codec(s, st, NULL)) < 0)
>                         return err;
>                 av_assert0(st->request_probe <= 0); // now 50
> SIGABRT
>

Can you actually make that happen, or is that just speculation?

- Hendrik
Andreas Cadhalpun Oct. 18, 2016, 10:05 p.m. UTC | #4 
On 18.10.2016 23:46, Hendrik Leppkes wrote:
> On Tue, Oct 18, 2016 at 11:26 PM, Andreas Cadhalpun
> <andreas.cadhalpun@googlemail.com> wrote:
>> On 18.10.2016 22:56, Michael Niedermayer wrote:
>>> On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
>>>> Nothing guarantees to set request_probe to -1, so this assert can be
>>>> triggered, e.g. if st->probe_packets is 0.
>>>
>>> probe_codec() called with NULL should cause
>>> st->probe_packets = 0
>>> st->request_probe = -1;
>>
>> Yes, but request_probe can be change to a different value later on,
>> e.g. in ff_parse_mpeg2_descriptor:
>>
>> int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
>> {
>> [...]
>>             if (s->internal->raw_packet_buffer_remaining_size <= 0)
>>                 if ((err = probe_codec(s, st, NULL)) < 0) // probe_packets = 0, request_probe = -1
>>                     return err;
>> [...]
>>         ret = s->iformat->read_packet(s, pkt);
>> ~~~
>> ff_parse_mpeg2_descriptor([...])
>> {
>> [...]
>>     switch (desc_tag) {
>> [...]
>>     case 0x05: /* registration descriptor */
>> [...]
>>                 st->request_probe = 50;
>> [...]
>> }
>> ~~~
>> [...]
>>                 if (st->probe_packets) // still 0
>>                     if ((err = probe_codec(s, st, NULL)) < 0)
>>                         return err;
>>                 av_assert0(st->request_probe <= 0); // now 50
>> SIGABRT
>>
> 
> Can you actually make that happen, or is that just speculation?

Yes, at least in ffmpeg 3.1.4 and master with commit 04fa20d reverted.
(I do fuzz-testing, not speculating.)

Best regards,
Andreas
Michael Niedermayer Oct. 19, 2016, 3:29 a.m. UTC | #5 
On Tue, Oct 18, 2016 at 11:26:24PM +0200, Andreas Cadhalpun wrote:
> On 18.10.2016 22:56, Michael Niedermayer wrote:
> > On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
> >> Nothing guarantees to set request_probe to -1, so this assert can be
> >> triggered, e.g. if st->probe_packets is 0.
> > 
> > probe_codec() called with NULL should cause
> > st->probe_packets = 0
> > st->request_probe = -1;
> 
> Yes, but request_probe can be change to a different value later on,
> e.g. in ff_parse_mpeg2_descriptor:
> 
> int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
> {
> [...]
>             if (s->internal->raw_packet_buffer_remaining_size <= 0)
>                 if ((err = probe_codec(s, st, NULL)) < 0) // probe_packets = 0, request_probe = -1
>                     return err;
> [...]
>         ret = s->iformat->read_packet(s, pkt);
> ~~~
> ff_parse_mpeg2_descriptor([...])
> {
> [...]
>     switch (desc_tag) {
> [...]
>     case 0x05: /* registration descriptor */
> [...]
>                 st->request_probe = 50;
> [...]
> }
> ~~~
> [...]
>                 if (st->probe_packets) // still 0
>                     if ((err = probe_codec(s, st, NULL)) < 0)
>                         return err;
>                 av_assert0(st->request_probe <= 0); // now 50
> SIGABRT

hmm, i guess the patch is then ok
alternatively you could use i think:
@@ -803,7 +803,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
                 return ret;
             for (i = 0; i < s->nb_streams; i++) {
                 st = s->streams[i];
-                if (st->probe_packets)
+                if (st->probe_packets || st->request_probe > 0)
                     if ((err = probe_codec(s, st, NULL)) < 0)
                         return err;
                 av_assert0(st->request_probe <= 0);


[...]
diff mbox

Patch

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 8a51aea..a62a073 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -806,7 +806,6 @@  int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
                 if (st->probe_packets)
                     if ((err = probe_codec(s, st, NULL)) < 0)
                         return err;
-                av_assert0(st->request_probe <= 0);
             }
             continue;
         }