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 504493 - mp3parse should read channel mode, crc and actual frame bitrate
mp3parse should read channel mode, crc and actual frame bitrate
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-ugly
git master
Other All
: Normal enhancement
: 0.10.7
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2007-12-19 16:25 UTC by Thiago Sousa Santos
Modified: 2013-07-18 14:53 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Solves the problem (8.31 KB, patch)
2007-12-20 02:31 UTC, Thiago Sousa Santos
none Details | Review
updated patch (8.33 KB, patch)
2007-12-20 06:18 UTC, Thiago Sousa Santos
none Details | Review
update patch 2 (8.34 KB, patch)
2007-12-20 20:57 UTC, Thiago Sousa Santos
none Details | Review
another updated patch (7.88 KB, patch)
2008-01-31 14:59 UTC, Thiago Sousa Santos
committed Details | Review

Description Thiago Sousa Santos 2007-12-19 16:25:52 UTC
In its current state, mp3parse sends messages and events when the average bitrate of the frames changes.

But it should and could provide more info, such as the channel mode (joint, stereo, dual or mono), if the stream is using crc and send the bitrate of the frames when they change (not only when the average changes). This is needed by some applications.
Comment 1 Thiago Sousa Santos 2007-12-20 02:31:35 UTC
Created attachment 101292 [details] [review]
Solves the problem

This patches adds the verification of CRC, channel mode and bitrate (on each frame) and sends the tag event and message whenever one of those changes.

Any comments, refactorings, hints, tips and critics are welcome.
Comment 2 Thiago Sousa Santos 2007-12-20 02:32:50 UTC
(In reply to comment #1)
> Created an attachment (id=101292) [edit]
> Solves the problem
> 
> This patches adds the verification of CRC, channel mode and bitrate (on each
> frame) and sends the tag event and message whenever one of those changes.
> 
> Any comments, refactorings, hints, tips and critics are welcome.
> 

by "verification of CRC" I mean, it checks the frame header to see if the frame is using or not CRC, nothing more than that.
Comment 3 Thiago Sousa Santos 2007-12-20 06:18:59 UTC
Created attachment 101302 [details] [review]
updated patch

Came to the conclusion that the tag name for a channel mode should be "channel-mode" instead of just "mode".

Also tag for crc protection is now "has-crc" instead of just "crc".
Comment 4 Thiago Sousa Santos 2007-12-20 20:57:56 UTC
Created attachment 101357 [details] [review]
update patch 2

Forgot to rename the constants
Comment 5 Thiago Sousa Santos 2008-01-22 16:55:57 UTC
Testing my patch with VBR mp3 files and I'm starting to wonder if sending tag events for each frame bitrate might not be a good idea. It can push one event per frame.
Comment 6 Jan Schmidt 2008-01-24 19:41:06 UTC
Indeed - with a VBR stream, the bitrate is almost certainly going to change on every frame. Is it still useful for you to have the other info without knowing the bitrate all the time?
Comment 7 Sebastian Dröge (slomo) 2008-01-24 19:48:05 UTC
I'm not exactly sure what the use of the CRC and channel mode tags is. If a frame has a CRC checksum attached that's good to verify the correctness of the frame but not very useful for the world outside of the parsers and decoders. For the channel mode the same applies, IMHO the only useful information for the outside is whether this file is mono or stereo and this can be seen from the caps.

But OTOH it definitely won't hurt to post information about CRC and channel mode as tags... I only don't see the use of them.
Comment 8 Thiago Sousa Santos 2008-01-31 13:41:51 UTC
Well, a2dp (Advanced Audio Distribution Profile) needs to know that info for negotiating the stream connection with a remote bluetooth device. So either I need some element that provides me with that info or inspect the buffer inside the a2dpsink I'm writting, but I think that mp3parse is a better place to put mp3 header inspection code.

Anyway, I'll make a new patch without bitrate per frame.
Comment 9 Thiago Sousa Santos 2008-01-31 14:59:45 UTC
Created attachment 104108 [details] [review]
another updated patch

Removed bitrate tags per frame from the previous patch.
Comment 10 Sebastian Dröge (slomo) 2008-02-22 07:11:08 UTC
Ok, committed (with the two variable declarations in the middle of a block moved to the beginning... yay C89 :) )

Also changed the nicks from joint to joint-stereo and dual to dual-channels as this is how's it's called everywhere I looked ;)

2008-02-22  Sebastian Dröge  <slomo@circular-chaos.org>

	Patch by: Thiago Sousa Santos <thiagoss at lcc dot ufcg dot edu dot br>

	* gst/mpegaudioparse/gstmpegaudioparse.c:
	(gst_mp3_channel_mode_get_type),
	(mp3_type_frame_length_from_header), (gst_mp3parse_class_init),
	(gst_mp3parse_reset), (gst_mp3parse_emit_frame),
	(gst_mp3parse_chain):
	* gst/mpegaudioparse/gstmpegaudioparse.h:
	Post channel mode and CRC as tags. Fixes bug #504493.
Comment 11 Tim-Philipp Müller 2008-02-23 10:29:40 UTC
(Just for the record: I'm not entirely convinced this is a good idea. Also, we might have to change this again if it turns out that we're posting silly amounts of tag messages on the bus in some cases.).
Comment 12 Daniel Benoy 2009-02-24 20:58:00 UTC
The bluez bluetooth stack version 3.36 a2dp gstreamer plugin appears to expect to see 'joint' and 'dual', rather than 'joint-stereo' and 'dual-channels'.  So it fails.

Is that a problem with gstreamer, or should I submit this issue to the bluez team?
Comment 13 Jan Schmidt 2009-02-25 08:30:38 UTC
I say it should change in a2dp if it needs to:
a) joint-stereo and dual-channels are more descriptive and I like them better
b) we don't change such things in gst-plugins-ugly once they've been released and are considered 'public API'
Comment 14 Tim-Philipp Müller 2009-02-26 09:06:12 UTC
Note that different elements seem to post different tags in this case, e.g. the "mad" element will post mode tags with strings like "mono", "dual" or "joint", whereas mp3parse will post what you've described ..
Comment 15 Daniel Benoy 2009-02-26 14:38:54 UTC
Alright.  So bluez should be expected to handle both?

If that's the case, then I'll file a bug report with the bluez team.
Comment 16 David Schleef 2010-01-15 05:31:16 UTC
I don't agree that the channel mode is useful to a2dp.  The spec clearly states (4.4.2.3) that:

  "Table 4.10 shows the value of Channel Mode field for MPEG-1,2 Audio. For the
decoder in the SNK all features shall be supported."

Moreover, the channel mode in mp3 can change from packet to packet, and some encoders actually do this.  This causes a flood of tags.
Comment 17 Sebastian Dröge (slomo) 2011-05-18 20:47:42 UTC
So what should happen here now?
Comment 18 Tobias Mueller 2011-07-08 12:15:43 UTC
Reopening as I can't see any open non-developer question.

Anyway, I guess with the committed patch this particular bug report should be set to FIXED and for any issues that follow up, a new bug report should be opened.
Comment 19 Edward Hervey 2013-07-18 14:53:39 UTC
Issue is now with bluez (and nobody seems to have bothered for 3 years). Initial issue is fixed.