Local track bitrate missing

[kingosticks]

Where is the bitrate field meant to come from for local tracks? Should it be in the tag cache? It doesn't feature in my tag_cache file or in _convert_mpd_data(). I only notice this as the status response contains "bitrate: None" for local tracks, which gives my client an exception when parsing the response as it expected an integer.

[ZenithDK]

As far I as remember without looking at the code - there is a ID3v2 tag for bitrate - and I have seen this filled in on my FLACs. The models.py file has a property for bitrate on the track, so it should be possible to parse and store it.

I was going to take a look at this next and implement the things for "status" that are currently missing, but haven't started yet.

[kingosticks]

Yeh I see the field in models (and that's what mopidy-spotify sets to make it work), just can't find anywhere that populates it for the local backend.

[ZenithDK]

Yeah, well it doesn't - so you are right :-)
Look in the file named scan.py under the audio directory - that's where the track parsing happens.
This needs to be added - seeing how I have added a ton of other new tags recently, this should be fairly easy.
But this is different from what is being output using status, though - from my understanding.
I think that needs to be parsed when you start playing the track.
Nothing uses the bitrate that you parse from the track tags - so not sure what the use is (maybe the bitrate in status?).

[kingosticks]

The bitrate field of the status command response comes directly from the track bitrate ala https://github.com/mopidy/mopidy/blob/develop/mopidy/frontends/mpd/protocol/status.py#L218
I think it just needs including in audio_data_to_track()

[ZenithDK]

Yeah, I think you are right then - should be pretty straightforward in that case.

[jodal]

Huhum... AFAICT from PR #559 mr @ZenithDK himself has fixed bitrate extraction when scanning already. :-)

[kingosticks]

Indeed he has, which is great. However I think the mpd frontend should still print 0 (or omit the line entirely from the output) rather than "None" when the track's bitrate is missing. Yay or nay?

Also: I don't see a test for the bitrate field in that PR. Presumably that should be added.

[jodal]

What does MPD do?

[ZenithDK]

I forgot about that one-liner - but having thought about it today I don't think this is the right way to go.
MPD does not put bitrates into its tag_cache, I think it looks up the bitrate when it starts playing back the file, which is the way I think we should go as well.
I don't think the bitrate tag is used for anything, all players probably queries the "status" to get the bitrate, samplerate, and channels.

[kingosticks]

The MPD tag cache doesn't include the bitrate, the decoders calculate it on the fly as the track is played.
i.e. for flac

bit_rate = nbytes * 8 * frame->header.sample_rate /
            (1000 * frame->header.blocksize);

If no song is currently playing they don't print the bitrate, time, elapsed or audio lines at all.

if (player_status.state != PlayerState::STOP) {
        client_printf(client,
                  COMMAND_STATUS_TIME ": %i:%i\n"
                  "elapsed: %1.3f\n"
                  COMMAND_STATUS_BITRATE ": %u\n",
                  (int)(player_status.elapsed_time + 0.5),
                  (int)(player_status.total_time + 0.5),
                  player_status.elapsed_time,
                  player_status.bit_rate);

        if (player_status.audio_format.IsDefined()) {
            struct audio_format_string af_string;

            client_printf(client,
                      COMMAND_STATUS_AUDIO ": %s\n",
                      audio_format_to_string(player_status.audio_format,
                                 &af_string));
        }
    }

[jodal]

We could probably do it that way for local tracks, but it won't work for i.e. Spotify since it provides us with raw audio data, so we no longer have any information about the bit rate when the audio is played.

What's wrong with just keeping it as is? Matching the MPD tag cache isn't a goal in itself.

[kingosticks]

What spotify does is fine and what local tracks (now) do is fine too. I've got no issue with wherever it comes from, I just want the case where it's not set to be handled better in MPD's status command.

[ZenithDK]

@jodal Agree, but you asked and I answered ;)
"I think it looks up the bitrate when it starts playing back the file, which is the way I think we should go as well."

I don't think we should change what I have implemented, but conversely, I think getting it on the fly for local tracks is better (or should be used as a fall-back if the bitrate tag was not found).

[jodal]

Leaving this issue open until we've tested what happens if bitrate is missing. We probably don't want bitrate: None in the status response. I'm guessing no bitrate at all or bitrate: 0 is more correct according to MPD.

[kingosticks]

Sorry, are you saying to test what mpd does when bitrate is missing, or
mopidy?
On Nov 15, 2013 7:26 AM, "Stein Magnus Jodal" notifications@github.com
wrote:

Leaving this issue open until we've tested what happens if bitrate is
missing. We probably don't want bitrate: None in the status response. I'm
guessing no bitrate at all or bitrate: 0 is more correct according to MPD.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/577#issuecomment-28551698
.

[jodal]

The MPD server, not Mopidy.

[kingosticks]

But it depends on what the individual audio decoder being used wants to do.
Although certainly it'll be an integer. I guess I could try and find a
decoder that doesn't set it.... But 0 is the sensible answer as you say.
On Nov 15, 2013 8:32 AM, "Stein Magnus Jodal" notifications@github.com
wrote:

The MPD server, not Mopidy.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/577#issuecomment-28554155
.

[adamcik]

#508 could mean that we can have the actually bitrate from gstreamer available, that is if #508 is solved in the correct way at least :-)

[jodal]

I checked the MPD source. Bitrate is an uint16, printed as %u in the status response, thus We'll make sure to return 0 instead of None.