[MPlayer-advusers] [BUG] mencoder floating point exception with -of lavf

[Nico Sabbi]

Corey Hickey wrote:

>Here is the third version of my buffering muxer wrapping patch. I doubt
>it is acceptable in its current form, but it is mostly working for me
>and I should be able to finish it based on your feedback. I've come up
>with about 45 regression tests, the results of which I'll list in detail
>at the end of this email.
>
>This is going to be long, but I'd appreciate any help you can give me.
>
>
>----------------------------------------------------------------------
>First, here's a summary of the current status of each muxer:
>
>  
>

>muxer_mpeg.c:
>The counter code was distributed among different functions, and I hacked
>it out with a dull knife. As a result, not all the regression tests are
>the same. This part is definitely not finished, but if anyone reading
>this knows the mpeg muxer well I'd be interested in hearing how
>atrocious my changes are and what could be done better.
>  
>

later

>muxer_lavf.c:
>The counter removal here was simple and the diff is small. Since this
>muxer was broken before, I can't compare my regression tests to
>anything. Many tests encode and play fine. I suspect that the failures
>  
>

you can compare it to 1.0pre7

>----------------------------------------------------------------------
>The muxer buffer in this patch has quite a few differences to the one in
>my previous patches. I ran into a few problems with that design. The new
>design is much more complicated, for these reasons. Perhaps there's a
>better way.
>
>1. I can't just save the pointer to the stream buffer (and give it a new
>pointer) because the pointers change in other parts of mencoder. So, I
>have to use memcpy().
>
>2. s->buffer often holds data past s->buffer+len. So, I can't memcpy
>only len bytes.
>
>...and...
>
>3. I can't depend on s->buffer pointing to memory as big as
>s->buffer_size because functions like ds_get_packet() change the pointer
>to point to a smaller memory allocation.
>
>...and...
>
>4. s->buffer_len isn't always set. So, I have to take and store the
>maximum of len and s->buffer_len, and use that value for memcpy()ing the
>necessary number of bytes.
>
>5. The data in the audio buffer can change between the time the muxer
>receives an audio frame and the time the muxer receives a video frame.
>So, I have to make sure I don't overwrite the current audio buffer with
>data from the most recently buffered frame.
>
>6. The lavf muxer uses s->timer to set pts, so I have to store that too.
>Otherwise, the pts for all buffered packets would end up being the same.
>
>
>----------------------------------------------------------------------
>I have several questions by now:
>
>1. My patch makes muxer_write_chunk() buffer up to 100 frames of audio.
>  
>

>I picked that number arbitrarily, and I'd like to choose one that is
>large enough to be safe for all circumstances. So far 27 is the largest
>number of initial audio frames I've seen, but I can't really conjecture
>what would be best. My code allocates memory for storing s->buffer as
>required, so very little extra memory would be used if we pick a very
>large number. If nobody gives me any input here I'm going to choose 500
>as a "reasonable amount of overkill."
>  
>

bad: it has to buffer as much as needed. Buffers should be
realloc()ed and free()d as necessary

>2. muxer_mpeg.c doesn't set the counters in as straightforward a manner
>as all the other muxers; the counters in muxer_mpeg.c weren't updated
>for all frames. Putting the counter code in the wrapper implies that
>mencoder's timing is now based on what is sent to the muxer, and the
>muxer must keep up. I don't know if this is the correct way to go about
>it. In fact, I'm not comfortable with any of the changes I've made to
>muxer_mpeg.c.
>  
>

counters are increased the same as for any other muxer,
including fake (0 sized) video frames.
s->timer is the dts for every stream (equals pts for audio)

>3. In muxer_mpeg.c, s->gop_start is set but apparently never used. My
>patch removes it, but I'm not entirely comfortable about doing so. I
>don't know if gop_start is orphaned from previously removed code or if
>it's there for the convenience of a future feature.
>  
>

indeed it's not needed

>4. Will there ever be a situation wherein s->buffer is NULL and len is
>!= 0? My code will segfault if that ever happens. If there's any sane
>reason that could happen I can check for it and set len=0.
>  
>

I don't think so

>
>----------------------------------------------------------------------
>There are a couple small cleanup things to do later that shouldn't
>change functionality. Probably they should be separate patches.
>
>1. Either change messages like "Writing AVI header..." to non-AVI
>specific counterparts like "Writing container header...", or put the
>messages inside each individual muxer.
>  
>

yes, in each muxer

>2. muxer_avi.c writes the header three times. I think I can eliminate
>the second time.
>
>
>
>======================================================================
>
>----------------------------------------------------------------------
>-of mpeg
>
>---identical---
>test.avi -ovc lavc -lavcopts vcodec=mpeg2video -oac copy -of mpeg
>test.avi -ovc lavc -lavcopts vcodec=mpeg2video -oac mp3lame -of mpeg
>test.avi -ovc lavc -lavcopts vcodec=mpeg2video -nosound -of mpeg
>
>small.vob -ovc copy -oac mp3lame -of mpeg -ofps 24000/1001
>small.vob -ovc copy -nosound -of mpeg -ofps 24000/1001
>small.vob -ovc lavc -lavcopts vcodec=mpeg2video -oac mp3lame -of mpeg
>-ofps 24000/1001
>small.vob -ovc lavc -lavcopts vcodec=mpeg2video -nosound -of mpeg -ofps
>24000/1001
>
>mf://img/*.jpg -mf type=jpeg:fps=10 -ovc lavc -lavcopts
>vcodec=mpeg2video -of mpeg
>
>---different, but still play ok---
>test.avi -ovc copy -oac copy -of mpeg
>
>This example tries copying FMP4 video and mp3 using -of mpeg. This isn't
>supposed to work right, is it? 
>

it is

>Both patched and unpatched mplayer
>produce files with very bad a-v desync. The files seem identical when
>played, but the files are different. I think this is a result of
>duplicate frames -- mpegfile_write_chunk() immediately returns when
>s->buffer is null and never updates the timer.
>

uhm, maybe I forgot to add increase timer and dwLength

> Is that the correct
>behavior? avifile_write_chunk() updates the timer regardless. My
>muxer_write_chunk() function updates the timer always.
>  
>

you may be right

>
>small.vob -ovc lavc -lavcopts vcodec=mpeg2video -oac copy -of mpeg -ofps
>24000/1001
>
>The audio in the resulting files are both completely broken. For some
>reason, out of 1378308 bytes, the two differ by only two bytes:
>
>bugfood at bugfood:/tmp$ cmp -l old.mpeg new.mpeg
>     29 102 106
>   2077 102 106
>
>I suppose I should track this down; I have a feeling it's due to the
>timer differences again. I'll have a look later.
>
>
>----------------------------------------------------------------------
>-of lavf
>
>---can't compare to anything, but look ok---
>small.vob -ovc lavc -nosound -of lavf -lavfopts format=avi -ofps 24000/1001
>small.vob -ovc lavc -oac mp3lame -of lavf -lavfopts format=avi -ofps
>24000/1001
>small.vob -ovc lavc -oac copy -of lavf -lavfopts format=avi -ofps 24000/1001
>small.vob -ovc lavc -oac copy -of lavf -lavfopts format=mov -ofps 24000/1001
>small.vob -ovc lavc -oac copy -of lavf -lavfopts format=mp4 -ofps 24000/1001
>small.vob -ovc lavc -oac copy -of lavf -lavfopts format=asf -ofps 24000/1001
>small.vob -ovc lavc -oac copy -of lavf -lavfopts format=nut -ofps 24000/1001
>
>test.avi -ovc copy -oac copy -of lavf -lavfopts format=avi
>test.avi -ovc copy -oac copy -of lavf -lavfopts format=mov
>test.avi -ovc copy -oac copy -of lavf -lavfopts format=mp4
>
>
>---doesn't work yet, but might be bugs elsewhere---
>small.vob -ovc lavc -lavcopts vcodec=mpeg2video -oac copy -of lavf
>-lavfopts format=mpg -ofps 24000/1001
>
>Oddly, this muxes an avi file. I'll investigate that some other time.
>  
>

try to set -o beforehand

>
>/mnt/tmp/small.vob -ovc lavc -lavcopts vcodec=mpeg2video -oac copy -of
>lavf -lavfopts format=mpg -ofps 24000/1001 -o test.mpg
>
>Adding "-o test.mpg" seems to make lavf write an mpeg file; however, I
>get lots of messages like the following:
>[mpeg @ 0x906440]packet too large, ignoring buffer limits to mux it
>[mpeg @ 0x906440]buffer underflow
>
>
>
>  
>

>That's all for now, except for a quote from Rich:
>"Try it -- it really won't be difficult!"
>
>Yes it was. :)) The code was easy, but I've ended up spending hours with
>two parallel gdb sessions trying to figure out where and why my patched
>mencoder differed from the unpatched version. I learned a lot about gdb
>and mencoder.
>
>-Corey
>  
>
>  
>

I still find your patch more intrusive than I believe necessary.
Why don't you follow the same approach as in my patch?

1) buffer as many frames as needed until you have at least one frame for 
every muxer_stream
2) when you meet the above condition call muxer->fix_parameters() for 
every muxer_stream and
muxer->write_header() the first time, then flush the buffered frames 
calling muxer->write_chunk()
for every buffered frames in the same order as you stored them
3) now that mencoder is in "normal" mode you don't need to buffer frames 
anymore and
you can let it call muxer->write_chunk() as normal and 
muxer->write_header() the closure time.

You can implement all of the above without altering the single muxers: I 
would write
some wrapper code that simply derefererences the function pointer 
members at the right time


>-    // alter counters:
>-    if(s->h.dwSampleSize){
>-	// CBR
>-	s->h.dwLength+=len/s->h.dwSampleSize;
>-	if(len%s->h.dwSampleSize) printf("Warning! len isn't divisable by samplesize!\n");
>-    } else {
>-	// VBR
>-	s->h.dwLength++;
>-    }
>-    s->timer=(double)s->h.dwLength*s->h.dwScale/s->h.dwRate;
>-    s->size+=len;
>     // if((unsigned int)len>s->h.dwSuggestedBufferSize) s->h.dwSuggestedBufferSize=len;
> 
> }
>  
>
>------------------------------------------------------------------------
>
>  
>

I would prefer to set s->timer incrementally rather than with a plain 
assignment;
second, I'm not sure that everything will be always ok centralizing 
those assignments
(thinking of the future:) .

    Nico