[Ffmpeg-devel] [DRAFT] Add fact chunk to non-PCM wav

[Michael Niedermayer]

Hi

On Mon, Feb 12, 2007 at 11:16:20AM +0100, Michel Bardiaux wrote:
> Michael Niedermayer wrote:
> >Hi
> >
> >On Wed, Feb 07, 2007 at 06:07:05PM +0100, Michel Bardiaux wrote:
> >>Michel Bardiaux wrote:
> >>>Michael Niedermayer wrote:
> [snip]
> >>>>in the write packet function
> >>>>assert(avpacket->pts != AV_NOPTS_VALUE);
> >>>>context->maxpts= FFMAX(context->maxpts, avpacket->pts);
> >>>>context->minpts= FFMIN(context->minpts, avpacket->pts);
> >>>>
> >>>>and in write_trailer
> >>>>number_of_sample= av_rescale(context->maxpts - context->minpts, 
> >>>>avctx->sample_rate * (int64_t)avstream->time_base.num, 
> >>>>avstream->time_base.den);
> >>>>
> >>>>untested of course but it should work approximately that way ...
> >>>Thanks, it works, with a little correction: the duration of the last 
> >>>packet must be added to the difference in pts. I assumed the 
> >>>pkt->duration is in the same unit as the timestamps.
> >>>
> >>>>>>then run the regression tests and send a patch which updates the 
> >>>>>>checksums
> >>>>>Not clear how to proceed here, do you mean a subsequent patch, or in 
> >>>>>the same?
> >>>I havent forgotten to make test, and of course the changes breaks 
> >>>regression since it adds 12 bytes to every non-PCM wav. I will post the 
> >>>final version with new checksums when the rest is approved.
> >>>
> >>Oops, forgot the attachment.
> >>
> >
> >[...]
> >
> >>@@ -46,6 +49,12 @@
> >>     }
> >>     end_tag(pb, fmt);
> >> 
> >>+    if(s->streams[0]->codec->codec_tag != 0x01 /* hence for all other 
> >>than PCM */) {
> >>+        fact = start_tag(pb, "fact");
> >>+        put_le32(pb, 0);
> >>+        end_tag(pb, fact);
> >>+    }
> >
> >the fact chunk should be ommited if url_is_streamed() as we wont be able to
> >fill it later ...
> 
> As I read it, the MS spec does not make any exception about FACT being 
> mandatory. To me it means WAV is simply not intended to be streamed, and 
>  any attempt to stream a WAV should simply be killed at the very start. 
> Unless someone can show me an example where a WAV (non-PCM) is streamed?
> 
> Anyway, the existing code already used start_tag/end_tag for the 'fmt ' 
> chunk, at that uses url_fseek.

libavformat has a internal buffer and url_fseek() over a small distance
can be done with no "external" seeks, anyway just try
ffmpeg -i foobar -f wav - >test.wav
or something like that 


> 
> So my feeling is that is is simply not possible to stream a WAV 
> correctly, at least not with the current implementation.
> 
> [snip]
> 
> >>+    if(pkt->pts == AV_NOPTS_VALUE) {
> >>+        av_log(s, AV_LOG_ERROR, "wav_write_packet: NOPTS\n");
> >>+        return -1;
> >>+    }
> >
> >hmm, this is a little nasty behaviour for a missing pts value when muxing
> >in a format which doesnt really "use" pts ...
> >i would prefer if we just ignore such packets in the min/maxpts calculation
> 
> You're the one who proposed an assert for that, see at top!

hmm indeed, sorry, but i still would suggest

if(pkt->pts != AV_NOPTS_VALUE){
    context->maxpts= FFMAX(context->maxpts, pkt->pts);
    context->minpts= FFMAX(context->minpts, pkt->pts);
}else
    av_log(s, AV_LOG_INFO, "wav_write_packet: NOPTS\n");


> 
> BTW I feel that the 'assert' should be hunted down even more ruthlessly 
> as the dprintf. What do you think?

some asserts are inappropriate they surely should be hunted down, others
are valid and should not

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070212/c94b73e1/attachment.pgp>