After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 334748 - [faad] doesn't take sampling rate from demuxer
[faad] doesn't take sampling rate from demuxer
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 0.10.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 321351 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2006-03-16 11:32 UTC by Michael Smith
Modified: 2006-05-03 21:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Ugly hack? (2.68 KB, patch)
2006-05-02 19:57 UTC, Michael Smith
none Details | Review
Fix previous (2.68 KB, patch)
2006-05-02 20:28 UTC, Michael Smith
committed Details | Review

Description Michael Smith 2006-03-16 11:32:55 UTC
On some clips (I have a serenity trailer in H.264/AAC as a sample), the demuxer says the audio is 48000Hz (which is 'correct'), but the actual audio bitstream presumably says it's 44100.

FAAD takes this exclusively from the audio bitstream, so plays it back at the wrong rate.

It should take a rate from the demuxer if the demuxer provides this info.

I'll fix this up, this is just to remind me to do it when I have some time.
Comment 1 Tim-Philipp Müller 2006-03-16 12:57:56 UTC
Why should it take the rate from the demuxer?

It's not apparently obvious to me why the demuxer would be more likely to be correct than the stream header. FWIW, we had similar issues with the pixel-aspect-ratio of videos, and there we decided to use the PAR from the codec if in doubt.

Maybe we need to check the timestamps to decide which rate to use if both rates differ?

The file from bug #321351 also looks like a possible candidate for this bug IIRC.
Comment 2 Michael Smith 2006-03-16 13:11:40 UTC
Well, we have examples (the serenity trailer I mentioned, and the bug #321351 file also has an identical problem) where the demuxer has the correct sample rate, but the decoder is wrong. Checking timestamps is pretty nasty - we'd start off wrong, then decide to change sample rate after seeing a few packets? Also, would be hard to get right.

Do we have any examples of the opposite (demuxer wrong, decoder right) for this case at all? If there are, then things are indeed more complex...
Comment 3 Jan Schmidt 2006-03-16 13:13:34 UTC
If we do have any examples, noone's likely to have noticed, because they'll currently be playing correctly...
Comment 4 Michael Smith 2006-03-16 13:13:57 UTC
*** Bug 321351 has been marked as a duplicate of this bug. ***
Comment 5 Michael Smith 2006-03-17 12:03:26 UTC
The superman HD trailer from apple is even more problematic. There, the demuxer says it has 3 channels (unlikely to be right!), and is 1 Hz (certainly wrong). This is most likely a qtdemux bug?

The decoder seems to think it's 8 channels, which also seems unlikely.

So, this is more complex. Deferring for now. Need to think about it some more.
Comment 6 Tim-Philipp Müller 2006-04-21 11:43:47 UTC
faad is in -bad, not -ugly, moving there.
Comment 7 Tim-Philipp Müller 2006-05-02 17:43:30 UTC
Michael, do you still have problems with those files with the latest fixes to faad in -bad CVS?
Comment 8 Michael Smith 2006-05-02 19:14:20 UTC
Yes, still problematic. Here's some gst-launch output with -v:

/playbin0/decoder/faad0.sink: caps = audio/mpeg, mpegversion=(int)4, framed=(boolean)true, rate=(int)48000, channels=(int)2
/playbin0/decoder/faad0.src: caps = audio/x-raw-int, endianness=(int)1234, signed=(boolean)true, width=(int)16, depth=(int)16, rate=(int)44100, channels=(int)2, channel-positions=(GstAudioChannelPosition)< GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT, GST_AUDIO_CHANNEL_POSITION_FRONT_RIGHT >

Note that FAAD is overriding the '48000' on its sinkpad with '44100' from (presumably) the AAC data. It sounds wrong, as a result. Mplayer sounds better (and plays back at 48000Hz).

Hrm... faad debug output:
LOG   (0x8214c38 - 0:00:00.916956000)                 faad( 1227) gstfaad.c(320):gst_faad_setcaps:<faad0> created fake codec data (48000,2)
LOG   (0x8214c38 - 0:00:00.916985000)                 faad( 1227) gstfaad.c(1086):gst_faad_chain:<faad0> Timestamp on incoming buffer: 0:00:00.000000000, next_ts: 0:00:00.000000000
DEBUG (0x8214c38 - 0:00:00.917005000)                 faad( 1227) gstfaad.c(1110):gst_faad_chain:<faad0> initialising ...
DEBUG (0x8214c38 - 0:00:00.917217000)                 faad( 1227) gstfaad.c(1114):gst_faad_chain:<faad0> faacDecInit() ok: rate=44100,channels=2

Fake codec data at 48000, but after initialisation it's at 44100? Bug in the fake codec data generator?
Comment 9 Michael Smith 2006-05-02 19:57:24 UTC
Created attachment 64686 [details] [review]
Ugly hack?

Here's a proposed patch. It fixes the apple trailers for me.

However, I don't know if it breaks anything else - I suppose it might, in complex cases - but do those actually exist in real files? If neccesary, I can make it do a much more complete parse of the AAC header.

How do your samples go with this one, Tim?
Comment 10 Michael Smith 2006-05-02 20:28:16 UTC
Created attachment 64688 [details] [review]
Fix previous

Fix a shift to shift by the right amount.
Comment 11 Tim-Philipp Müller 2006-05-03 09:18:33 UTC
Patch works fine with my collection of tricky faad files (I did get an 'Internal Flow Error' with three times, but those files all worked when trying a second time and I think those errors are unrelated to the patch).

 http://movies.apple.com/movies/fox/x-men_3/x-men_3-pre_teaser_h720p.mov

works fine for me as well with the patch.


The patch does break unframed .aac file playback, but that's an easy fix as talked about on IRC:

<MikeS> I think with this: if (looks_like_valid_header(input_data, input_size) && faad->packetised) {
<MikeS> I actually meant: if (looks_like_valid_header(input_data, input_size) || !faad->packetised) {

Comment 12 Michael Smith 2006-05-03 10:26:38 UTC
Thanks for checking, Tim. Committed with the discussed change.