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.

(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.

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".

Created attachment 101357 [details] [review]
update patch 2

Forgot to rename the constants

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.

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?

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.

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.

Created attachment 104108 [details] [review]
another updated patch

Removed bitrate tags per frame from the previous patch.

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.

(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.).

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?

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'

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 ..

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.

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.

So what should happen here now?

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.

Issue is now with bluez (and nobody seems to have bothered for 3 years). Initial issue is fixed.