[Ffmpeg-devel] ffplay broken in latest svn

[Nikns Siankin]

On Tue, Jan 09, 2007 at 12:35:57PM -0000, Wolfram Gloger wrote:
>> >> -    double duration_error[MAX_STREAMS][MAX_STD_TIMEBASES]={{0}}; //FIXME malloc()?
>> >> +    double (*duration_error)[MAX_STD_TIMEBASES] = av_mallocz(MAX_STREAMS * MAX_STD_TIMEBASES * sizeof(duration_error));
>> >
>> >Ahem, this, without any further pointer initialization, cannot
>> >possibly be correct, the first duration_error[i][j] dereference will
>> >crash, also I think the sizeof(duration_error) (==
>> >MAX_STD_TIME_BASES*sizeof(double*) in this case) is probably
>> >unintentionally large..
>> 
>> sizeof(duration_error) != MAX_STD_TIME_BASES*sizeof(double*)
>> sizeof(duration_error) == sizeof(double*)
>
>You're basically right here (discounting possible but improbable size
>difference between "pointer to array" and "pointer to first element";
>I missed the indirection), but that is _even worse_ as usually
>sizeof(double*)<sizeof(double)!
>
>> Infact, it seems possibly correct ;]
>> http://c-faq.com/aryptr/dynmuldimary.html
>
>..but then you would have to change the declaration to
>
>double (*duration_error)[MAX_STREAMS][MAX_STD_TIMEBASES] = ...;
>
>and _every single access_ to (*duration_error)[i][j] which your patch
>certainly didn't do.

Form (*duration_error)[i][j] is only needed for above declaration.
AW, here is patch for such declaration:

--- libavformat/utils.c.orig	Tue Jan  2 22:35:46 2007
+++ libavformat/utils.c	Tue Jan  9 14:47:03 2007
@@ -1784,7 +1784,7 @@ int av_find_stream_info(AVFormatContext 
     AVPacketList *pktl=NULL, **ppktl;
     int64_t last_dts[MAX_STREAMS];
     int duration_count[MAX_STREAMS]={0};
-    double duration_error[MAX_STREAMS][MAX_STD_TIMEBASES]={{0}}; //FIXME malloc()?
+    double (*duration_error)[MAX_STREAMS][MAX_STD_TIMEBASES] = av_mallocz(sizeof(*duration_error));
 
     for(i=0;i<ic->nb_streams;i++) {
         st = ic->streams[i];
@@ -1896,12 +1896,12 @@ int av_find_stream_info(AVFormatContext 
 //                if(st->codec->codec_type == CODEC_TYPE_VIDEO)
 //                    av_log(NULL, AV_LOG_ERROR, "%f\n", dur);
                 if(duration_count[index] < 2)
-                    memset(duration_error, 0, sizeof(duration_error));
+                    memset(duration_error, 0, sizeof(*duration_error));
                 for(i=1; i<MAX_STD_TIMEBASES; i++){
                     int framerate= get_std_framerate(i);
                     int ticks= lrintf(dur*framerate/(1001*12));
                     double error= dur - ticks*1001*12/(double)framerate;
-                    duration_error[index][i] += error*error;
+                    (*duration_error)[index][i] += error*error;
                 }
                 duration_count[index]++;
 
@@ -1968,7 +1968,7 @@ int av_find_stream_info(AVFormatContext 
                 best_error= best_error*best_error*duration_count[i]*1000*12*30;
 
                 for(j=1; j<MAX_STD_TIMEBASES; j++){
-                    double error= duration_error[i][j] * get_std_framerate(j);
+                    double error= (*duration_error)[i][j] * get_std_framerate(j);
 //                    if(st->codec->codec_type == CODEC_TYPE_VIDEO)
 //                        av_log(NULL, AV_LOG_ERROR, "%f %f\n", get_std_framerate(j) / 12.0/1001, error);
                     if(error < best_error){
@@ -2016,6 +2016,9 @@ int av_find_stream_info(AVFormatContext 
         }
     }
 #endif
+
+    av_freep(&duration_error);
+
     return ret;
 }
 


>
>I stand by my claim that the patch is obviously incorrect.
>
>Regards,
>Wolfram.
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel at mplayerhq.hu
>http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel