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 776038 - audioringbuffer: do not require 4 byte multiples for encoded audio
audioringbuffer: do not require 4 byte multiples for encoded audio
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-12-13 09:47 UTC by Vincent Penquerc'h
Modified: 2016-12-13 11:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
audioringbuffer: do not require 4 byte multiples for encoded audio (1.34 KB, patch)
2016-12-13 09:47 UTC, Vincent Penquerc'h
accepted-commit_now Details | Review

Description Vincent Penquerc'h 2016-12-13 09:47:47 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 1 Sebastian Dröge (slomo) 2016-12-13 09:58:20 UTC
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
Comment 2 Vincent Penquerc'h 2016-12-13 10:10:11 UTC
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 3 Sebastian Dröge (slomo) 2016-12-13 10:13:36 UTC
Comment on attachment 341862 [details] [review]
audioringbuffer: do not require 4 byte multiples for encoded audio

Go for it then
Comment 4 Vincent Penquerc'h 2016-12-13 10:18:25 UTC
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
Comment 5 Tim-Philipp Müller 2016-12-13 10:20:03 UTC
Erm, did you check the payloading spec(s) to see if it says anything there?
Comment 6 Sebastian Dröge (slomo) 2016-12-13 10:29:05 UTC
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.
Comment 7 Tim-Philipp Müller 2016-12-13 10:50:31 UTC
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)
Comment 8 Vincent Penquerc'h 2016-12-13 10:53:56 UTC
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 ?
Comment 9 Tim-Philipp Müller 2016-12-13 11:05:19 UTC
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.
Comment 10 Vincent Penquerc'h 2016-12-13 11:30:53 UTC
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.
Comment 11 Tim-Philipp Müller 2016-12-13 11:40:03 UTC
Ok, great. Apologies for the alarmism :)