GNOME Bugzilla – Bug 776038
audioringbuffer: do not require 4 byte multiples for encoded audio
Last modified: 2016-12-13 11:40:03 UTC
Created attachment 341862 [details] [review] audioringbuffer: do not require 4 byte multiples for encoded audio I this this makes sense, since encoded audio does not have a concept of "frames", let alone constant bytes per frame. It fixes a problem I have in some proprietary (I know :/) code which wants to play encoded audio, but the sink rejects some packets here: size = gst_buffer_get_size (buf); if (G_UNLIKELY (size % bpf) != 0) goto wrong_size; With bpf to 1, it all works fine. Comments ? Objections ?
Comment on attachment 341862 [details] [review] audioringbuffer: do not require 4 byte multiples for encoded audio Did you do some archaeology to find out why this was 4 to begin with? Seems to make no sense but there might've been a reason
ocrete did: 19:26 < v> The iffy change is changing bpf to 1. But I think it's actually OK... 19:26 < ocrete> think you can make a cleaned up mp3 only patch and submit it through phab ? 19:26 < v> Not sure if it's rationalization thoug h:) 19:27 < ocrete> why change bpf to 1 ? 19:27 < v> Even if it's possibly buggy ? 19:27 < ocrete> I didnt see the bpf change 19:27 < ocrete> why is that ? 19:27 < v> bpf to 1 otherwise baseaudiosink says "wrong size!" and error sout. 19:28 < ocrete> because the input is not in 4-byte quanta ? 19:28 < v> For mp3, it was always N bytes, with N about 400 or so, and divisible by 4. Except for the first packet, which was one byte less. 19:28 < ocrete> the bpf bit worries me.. I'm not sure what pulsesink expects (or what bluetooth expects) 19:28 < v> (coming from mpegaudioparse) 19:28 < ocrete> oh from mpegaudioparse 19:28 < ocrete> hmm 19:29 < ocrete> would possibly make sense to have an upstream patch and get some more feedback 19:29 * ocrete as no idea about the secrets of the mp3 format ;) 19:29 < v> Yes. It's encoded data after all, so bytes-per-frame has no real meaning... 19:29 < v> I guess I'll do that. 19:29 < ocrete> all of the encoded formatts seem to have bpf=4 19:30 < v> Could be cut and paste. I'd totally do that :D 19:30 < v> And for Vorbis it's clear-cut wrong. 19:30 < v> Assuming it really is what I think it is. 19:41 < ocrete> the origin of that 4 is 19:41 < ocrete> spec->type = GST_BUFTYPE_MPEG; 19:41 < ocrete> - spec->format = GST_MPEG; 19:41 < ocrete> - spec->width = 16; 19:41 < ocrete> - spec->depth = 16; 19:41 < ocrete> - spec->channels = 2; 19:41 < ocrete> + spec->info.bpf = 4; 19:42 < ocrete> I think it'S completely made up
Comment on attachment 341862 [details] [review] audioringbuffer: do not require 4 byte multiples for encoded audio Go for it then
Thanks, pushed. commit 6ee5922f2f93e5e24a68f2c58859b3474920c23b Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Tue Dec 13 09:44:09 2016 +0000 audioringbuffer: do not require 4 byte multiple for encoded MPEG Bytes per frame doesn't make sense for encoded audio. https://bugzilla.gnome.org/show_bug.cgi?id=776038
Erm, did you check the payloading spec(s) to see if it says anything there?
This is independent of payloading. If IEC 61937 payloading is required, the API for that has gst_audio_iec61937_frame_size() to get the actually required framesize and the payloading functions will have to ensure that everything is of the correct size. For raw MP3/AAC, a multiple of 4 bytes makes no sense.
Ok, I was mostly concerned about the IEC payloading scenario, didn't realise this was 'raw'. I trust this doesn't affect the iec payloading scenario then (frames still need to start at 4-byte boundaries and not be positioned "somewhere" in the ring buffer according to some timestamp calculations and bpf)
I've no idea what IEC payloading is, so I don't know whether that would break it. Any quick pipeline which I could check with ?
When you output (encoded) audio via SPDIF, you'd probably need a suitable device/card for that, don't know if there's an easy way to check without.
There is a audio/x-iec958 case at the same level as the encoded formats that I changed. Assuming this is the format you're talking about, it seems that this one uses bpf 4, and so should be unchanged.
Ok, great. Apologies for the alarmism :)