Bug 81905 – [GTK] WebAudio channelSize issue

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[sangho park]

Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:164
m_channelSize += GST_BUFFER_OFFSET_END(buffer) - GST_BUFFER_OFFSET(buffer);

in case of ogg file input without BOS information (e.g: http://labs.dinahmoe.com/ToneCraft/)
m_channelSize will be always '0'

so I think.. the code is needed to change like this,
m_channelSize += buffer->size * 0.25

i tested tonecraft page and succeed.

[Philippe Normand]

(In reply to comment #0)
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:164
> m_channelSize += GST_BUFFER_OFFSET_END(buffer) - GST_BUFFER_OFFSET(buffer);
> 
> in case of ogg file input without BOS information (e.g: http://labs.dinahmoe.com/ToneCraft/)
> m_channelSize will be always '0'
> 
> so I think.. the code is needed to change like this,
> m_channelSize += buffer->size * 0.25
> 

Hum, can you explain this please?

Maybe it'd be good to have a layout test for this issue.

[sangho park]

when i test "http://labs.dinahmoe.com/ToneCraft/" page,
gstreamer's 'oddgemux' plugin warns... "page is not BOS page, all streams identified".
as i know, most webaudio example page that contains ogg are same.

at this time, GST_BUFFER_OFFSET_END(buffer) and GST_BUFFER_OFFSET(buffer) were '0'. so i need a meaningful channel size.

what do you think about this situation?
hmm.. imo.. ogg wihout BOS information is not perfect file, but not evil...
and channel size can be calculated by size of buffer.

[Philippe Normand]

Sebastian, can you give some advice on this issue?

[sangho park]

Created attachment 133469 [details]
sample ogg file

[Philippe Normand]

I'll upload a patch in few minutes. Can you test it?

[sangho park]

sure I'm ready~~ :)

[Philippe Normand]

Created attachment 133472 [details]
Patch

[sangho park]

hmm.. i'm compiling now.. but

in AudioFileReaderGStreamer.cpp, line 151~156 doesn't need anymore, right?

[Philippe Normand]

(In reply to comment #8)
> hmm.. i'm compiling now.. but
> 
> in AudioFileReaderGStreamer.cpp, line 151~156 doesn't need anymore, right?

Thue, but it's unrelated to this patch. Can be cleaned up when I land this one though, looks harmless!

[sangho park]

(In reply to comment #5)
> I'll upload a patch in few minutes. Can you test it?

test done. looks great!

[Sebastian Dröge (slomo)]

Comment on attachment 133472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133472&action=review

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:166
> +    GstClockTime duration = gst_audio_duration_from_pad_buffer(sinkPad.get(), buffer);

This function is not in 0.11 anymore, just calculate the duration yourself from the buffer size and the caps (and in 0.11 the GstAudioInfo).

Other than that this is the correct thing to do, using the offset fields will only cause problems.

[Philippe Normand]

Created attachment 134022 [details]
Patch

[Sebastian Dröge (slomo)]

Comment on attachment 134022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134022&action=review

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:166
> +    if (!gst_structure_get_int(structure, "width", &width) || !width) {

Note that all this will only work with 0.10 but I guess that's fine as this code has to be ported to 0.11/1.0 anyway

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:172
> +    long bytes = GST_BUFFER_SIZE(buffer);

Better use guint64 or something similar here instead of long. For the calculation one line below you need a 64 bit integer to prevent overflows.

[Philippe Normand]

Comment on attachment 134022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134022&action=review

>> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:166
>> +    if (!gst_structure_get_int(structure, "width", &width) || !width) {
> 
> Note that all this will only work with 0.10 but I guess that's fine as this code has to be ported to 0.11/1.0 anyway

Yes.

>> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:172
>> +    long bytes = GST_BUFFER_SIZE(buffer);
> 
> Better use guint64 or something similar here instead of long. For the calculation one line below you need a 64 bit integer to prevent overflows.

Oh hum yes, actually I can just directly use GST_BUFFER_SIZE below.

[Philippe Normand]

Created attachment 134038 [details]
Patch

[Philippe Normand]

Martin can you have a look at this patch please? Sebastian did a first pass :)

[Philippe Normand]

Comment on attachment 134038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134038&action=review

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:172
> +    GstClockTime duration = (reinterpret_cast<guint64>(GST_BUFFER_SIZE(buffer)) * 8 * GST_SECOND) / (sampleRate * channels * width);

Oh hum that should be a static_cast.

[Philippe Normand]

Committed r112646: <http://trac.webkit.org/changeset/112646>