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

[Daniel Steinberg]

I dug deeper and discovered that we were running some code that had
been reinstated from an earlier (pre packet_buffer_end) ffmpeg
release.  Updating that code to follow the packet buffer semantics
seemed to have solved the problem.


On Mon, Mar 23, 2009 at 5:46 PM, Daniel Steinberg
<daniel at instantharmony.com> wrote:
> 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
>