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

[Corey Hickey]

Thanks for the feedback, Nico. I've been busy with other stuff but I should
have time to refine my patch based on your suggestions some time in the next
few days. There are a few things to which I wanted to respond, and in some
cases I need a bit of clarification.

>>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

Isn't it necessary to impose some kind of a limit with regard to how many
frames can be buffered? In the extreme case I'm imagining a hypothetical
(though perhaps unrealistic) file that has a video stream defined in the
header but actually has only audio frames. Without a limit, mencoder would
just chew up memory until it ran out or reached EOF.

Note that the only things my patch statically allocates are several arrays
of size MUXER_FRAME_BUFFER_SIZE. If I added it up correctly, it comes to
only 40 bytes per frame. I could easily modify my patch so that the memory
is free()d once muxer_write_chunk() stops buffering frames (I should have
done that before anyway).

The only reason I'm arguing this at all is that it seems more
straightforward to me to allocate the storage all at once rather than
realloc()ing each time data corresponding to another frame must be stored. I
don't have any strong feelings either way. If I'm mistaken about the
necessity of imposing a limit to the number of frames buffered, then all
this is moot anyway.

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

I'd be happy to make it more simple, if I can. I actually thought it would
be more simple -- I just kept finding more and more things I had to save for
each frame.

> 1) buffer as many frames as needed until you have at least one frame for
> every muxer_stream

I can do this, but is it necessary to wait for the audio? I thought the
characteristics of the audio to be encoded were known once
preinit_audio_filters() is called.

> 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

That's pretty much how my patch does it. The only weirdness is that I have
to temporarily alter some members of each muxer_stream_t when flushing the
buffered frames. ...and make sure the data is back to its original state
before muxer_write_chunk() returns.

> 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.

Yes. Once the buffered frames have been flushed then my muxer_write_chunk()
simply calls s->muxer->write_chunk() with the same arguments.

> 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

The only alteration to the individual muxers is to move the timing code into
the wrapper function -- the timer must be updated regardless of the
buffering status.

>>-    s->timer=(double)s->h.dwLength*s->h.dwScale/s->h.dwRate;

> I would prefer to set s->timer incrementally rather than with a plain
> assignment;

I'll look into that. It just occurred to me, though: wouldn't incrementing
s->timer allow rounding errors to accumulate since it's a double? Just a
thought; I'll investigate next time I'm at home.

Thanks again,
Corey