[Ffmpeg-devel] [PATCH] flacenc - md5

[Michael Niedermayer]

Hi

On Sun, Jul 09, 2006 at 01:50:18PM -0400, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> > Hi
> > 
> > On Sat, Jul 08, 2006 at 10:41:05PM -0400, Justin Ruggles wrote:
> > 
> >>sheesh. I do need my cola tonight. The regression references would need
> >>to be updated as well.  Here is a patch with regression updates.
> > 
> > 
> > all things you do in the flac muxer will be missing if flac is stored in
> > other containers, is raw flac more common then flac in ogg?
> > 
> > the problem i have with the changes is that theres lots of new code
> > but the md5 will still be missing if flac is stored in any container
> > furthermore the md5 isnt really mandatory so unless you can find a
> > simple way to set the md5 which then also works with other containers,
> > iam against setting it at all in lav*
> 
> Raw flac is not just more common, it is vastly more common.  I have only
> seen a few ogg-flac files out there, but raw flac is all over the place.
>  Like I said before, the reference encoder doesn't even put the MD5 in
> ogg-flac files.  I think writing it, even if only in raw flac, is still
> important.

ok


> 
> Concerning trying to get it into other containers though...maybe your
> previous suggestion of a 2-pass solution would work ok.  I would have to
> study the current system for reading/writing the 2-pass info file to
> know exactly how this could work though.  If it works how I think, it
> might be the simplest solution & could avoid passing the md5 through
> AVCodecContext or modifying the extradata after it has been written already.
> 
> As far as the other metadata, Ogg has to do things a bit differently
> anyway.  The standard flac-to-ogg mapping adds its own header to the
> streaminfo, then puts each metadata block into a separate ogg packet.
> 
> I could shrink & simplify the code quite a bit if the extradata is
> explicitly defined to contain only the streaminfo.  In fact, I like this
> idea.  It may limit some of the raw flac functionality, but it would be
> simpler for other containers since they would know exactly what they are
> writing.  I am still in favor of adding the padding and vorbis comment
> in raw flac though.  The vorbis comment writing could conceivably be put
> in a place where other muxers could access it if they wanted to...
> 
> > 
> > [...]
> > 
> >>    // keep current count of audio samples
> >>    if(pkt->pts != AV_NOPTS_VALUE) {
> >>        flac->sample_count = (float)(pkt->pts+pkt->duration) *
> >>                             (float)st->time_base.num /
> >>                             (float)st->time_base.den *
> >>                             (float)st->codec->sample_rate + 0.5;
> > 
> > 
> > use av_rescale(), floats are unacceptable and inaccurate
> 
> ok. I'll try it and see if I can get it to work. Doing the integer math
> directly, I kept getting 1 sample too short due to rounding of the pts
> and duration. Could there be a way of setting a better time_base to
> ensure single-sample accuracy?

hmm hmmmmmmmmmmm
timebase is 1/90000? it should be 1/sample_rate or something like that
see av_set_pts_info()


> 
> > 
> > 
> >>Index: ffmpeg.c
> >>===================================================================
> >>--- ffmpeg.c	(revision 5683)
> >>+++ ffmpeg.c	(working copy)
> >>@@ -1393,13 +1393,11 @@
> >>                             ret = 0;
> >>                             /* encode any samples remaining in fifo */
> >>                             if(fifo_bytes > 0 && enc->codec->capabilities & CODEC_CAP_SMALL_LAST_FRAME) {
> >>-                                int fs_tmp = enc->frame_size;
> >>                                 enc->frame_size = fifo_bytes / (2 * enc->channels);
> >>                                 if(fifo_read(&ost->fifo, (uint8_t *)samples, fifo_bytes,
> >>                                         &ost->fifo.rptr) == 0) {
> >>                                     ret = avcodec_encode_audio(enc, bit_buffer, bit_buffer_size, samples);
> >>                                 }
> >>-                                enc->frame_size = fs_tmp;
> >>                             }
> >>                             if(ret <= 0) {
> >>                                 ret = avcodec_encode_audio(enc, bit_buffer, bit_buffer_size, NULL);
> > 
> > 
> > this change seems unrelated ...
> > 
> > [...]
> 
> It is related to the calculation of the sample count.  The last packet
> was reporting an incorrect duration due to frame_size being set back to
> the larger size.  The result was that the sample count was too high.
> Leaving frame_size the actual size of the last frame seemed okay to me,
> especially since it is the last frame anyway.

ok, ive no objections to the ffmpeg.c change but it should be commited
seperately as its a bugfix which isnt related to the md5-flac thing

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is