Bug 81905 - [GTK] WebAudio channelSize issue
Summary: [GTK] WebAudio channelSize issue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-22 07:10 PDT by sangho park
Modified: 2012-03-30 00:46 PDT (History)
6 users (show)

See Also:


Attachments
sample ogg file (9.57 KB, video/ogg)
2012-03-23 06:13 PDT, sangho park
no flags Details
Patch (2.70 KB, patch)
2012-03-23 06:36 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (2.59 KB, patch)
2012-03-27 04:30 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (2.60 KB, patch)
2012-03-27 05:57 PDT, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sangho park 2012-03-22 07:10:17 PDT
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.
Comment 1 Philippe Normand 2012-03-22 07:26:08 PDT
(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.
Comment 2 sangho park 2012-03-22 07:51:30 PDT
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.
Comment 3 Philippe Normand 2012-03-22 08:27:04 PDT
Sebastian, can you give some advice on this issue?
Comment 4 sangho park 2012-03-23 06:13:41 PDT
Created attachment 133469 [details]
sample ogg file
Comment 5 Philippe Normand 2012-03-23 06:31:25 PDT
I'll upload a patch in few minutes. Can you test it?
Comment 6 sangho park 2012-03-23 06:32:49 PDT
sure I'm ready~~ :)
Comment 7 Philippe Normand 2012-03-23 06:36:10 PDT
Created attachment 133472 [details]
Patch
Comment 8 sangho park 2012-03-23 07:16:08 PDT
hmm.. i'm compiling now.. but

in AudioFileReaderGStreamer.cpp, line 151~156 doesn't need anymore, right?
Comment 9 Philippe Normand 2012-03-23 07:28:47 PDT
(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!
Comment 10 sangho park 2012-03-23 07:43:51 PDT
(In reply to comment #5)
> I'll upload a patch in few minutes. Can you test it?

test done. looks great!
Comment 11 Sebastian Dröge (slomo) 2012-03-27 03:46:48 PDT
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.
Comment 12 Philippe Normand 2012-03-27 04:30:24 PDT
Created attachment 134022 [details]
Patch
Comment 13 Sebastian Dröge (slomo) 2012-03-27 04:40:44 PDT
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.
Comment 14 Philippe Normand 2012-03-27 04:46:01 PDT
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.
Comment 15 Philippe Normand 2012-03-27 05:57:24 PDT
Created attachment 134038 [details]
Patch
Comment 16 Philippe Normand 2012-03-28 07:41:27 PDT
Martin can you have a look at this patch please? Sebastian did a first pass :)
Comment 17 Philippe Normand 2012-03-28 07:51:31 PDT
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.
Comment 18 Philippe Normand 2012-03-30 00:46:20 PDT
Committed r112646: <http://trac.webkit.org/changeset/112646>