[FFmpeg-devel] logic error in libavformat/utils.c ?

[Daniel Steinberg]

I believe there is a typo in libavformat/utils.c::av_read_frame()

           if(genpts && next_pkt->dts != AV_NOPTS_VALUE){
               while(pktl && next_pkt->pts == AV_NOPTS_VALUE){
                  . . .
               }
           }
           if(   next_pkt->pts != AV_NOPTS_VALUE
>>>         || next_pkt->dts == AV_NOPTS_VALUE
              || !genpts || eof){
               /* read packet from packet buffer, if there is data */
               *pkt = *next_pkt;
               s->packet_buffer = pktl->next;
               av_free(pktl);
               return 0;
           }
      }
     if (genpts){
         int ret = av_read_frame_internal(s, pkt);
         . . .

I encountered a consistent crash when AVFMT_FLAG_GENPTS was set and
the input (H.264) packets had valid dts, but no pts set.  At some point in the
first loop that tries to assign pts from dts, pktl->next becomes NULL and it
enters the 'if' in question.  Because next_pkt->dts is valid, the return is
not taken, and the if(genpts) block is entered, perhaps, erroneously.

At that point, s->packet_buffer_end == NULL, and the subsequent
call to add_to_pktbuf() crashes when it tries to dereference it.

Changing the test above to:

           if(   next_pkt->pts != AV_NOPTS_VALUE
>>>         || next_pkt->dts != AV_NOPTS_VALUE
              || !genpts || eof){

solves the crash problem, seems to result in correct packet parsing,
and causes no ill effects in a wide selection of other videos that i tried,
but it is hard to tell where the real fault lay.

I can supply some errant media, if necessary, but first i wanted to
run this by y'all.

On Sat, Mar 21, 2009 at 4:31 AM, Baptiste Coudurier
<baptiste.coudurier at gmail.com> wrote:

> Humm, I'm not maintainer of this code, but the code seems to add packets
> to the packet buffer only when genpts is set.
>
> Consequently, according to the loop computing pts for the packets, and
> assuming your packets have dts set, pts for the packet should be set at
> some time, unless the dts are not monotone which would cause the test:
> "next_pkt->dts < pktl->pkt.dts" to be false.

The first time through the loop, *next_pkt == pktl->pkt, so that test is false.
When pktl->next == 0, the code proceeds to the next test, which does not
pass (because pts == AV_NOPTS_VALUE and dts is valid).

> I believe the test for dts == AV_NOPTS_VALUE is because if dts is not
> set, pts cannot be set and function must return the packet nontheless.

Well, it's hard to tell, from the copious comments, but i thought the intention
here was for the packet to be returned *because* either there is a timestamp
in one of pts/dts OR timestamps are not being generated.

Perhaps the problem is really in add_to_pktbuf, which should be setting,
(*plast_pktl) instead of (*plast_pktl)->next, at least when *plast_pktl == NULL.


-Daniel Steinberg