The input buffer shouldn't be a member of the Decoder

[ceigel]

It is very rarely that the user of the mp3 decoder could load the entire mp3 file in memory and then call the Decoder function. Ideally the next_frame function would receive the input buffer. I was considering using this library for an embedded project. There the data would come from an SD card and the entire RAM of the device is 40k

[0x08088405]

Apologies for the delay, I've been extremely busy. I'll be honest, I mostly made this wrapper for myself when I realized that minimp3-rs allocated a Vec and cloned it for each frame and I wanted to use that in a real time application so that's a big no. It was meant to have seeking functionality which I have not implemented yet which is why it owns the data, also because convenience.

Anyways obviously this design won't work well for embedded, I'm thinking maybe I could rename the current one that does own it to DecoderStream or something and then add a Decoder with identical functions, except it takes in a &[u8] (being the data) instead of no arguments for next_frame, peek_frame etc. Would that work for you? I'd bump the version up of course since it's an API change.

[ceigel]

Hi. Well I don’t know how many people use this library but renaming stuff might make them disappointed. Maybe the new decoder should have a new name. But for now this seems to me the only thing that should change. In the meantime I created a version of the library for myself. Your version was heavily and shamelessly copied. :). I did not publish yet. It was also simplified as I didn’t care for float output. It is also very important for me to understand what is going on, since I want to get this in 40k of ram and do some ssd reading in between while letting the DMA and DAC do the playing. If you don’t have time I can eventually PR the changes in your repo.

[ceigel]

Hi, I have some update about my attempts at decoding mp3 on embedded devices. I discovered that the Decoder struct is big. The mp3dec_t struct is about 6k, the samples field also has about 2k. The device I work with (stm32f303vc) has a small high-speed ram section of 8k where stack is ideally placed. Which means that the Decoder has to be a static mut. Right now this is not possible since the constructor is not a const function. I've had some success with MaybeUninit for the mp3dec_t struct so I created a const constructor, but I need to call an init function on the decoder object. I can envisage a second struct that has a const constructor and holds the memory of the Decoder, which is eventually passed as an argument to the Decoder.

[0x08088405]

I wish GitHub notified me about this reply. As far as I know the init function just zeroes a few fields, it should be easy to reimplement as const, although hopefully by now you've had some success!

Edit: Yeah, here it is (header is unsigned char header[4])

void mp3dec_init(mp3dec_t *dec)
{
    dec->header[0] = 0;
}

Which means you can define a const fn like...

pub const fn new() -> Self {
    RawDecoder {
        ffi_frame: ffi::mp3dec_frame_info_t {
            frame_bytes: 0,
            channels: 0,
            hz: 0,
            layer: 0,
            bitrate_kbps: 0,
        },
        instance: ffi::mp3dec_t {
            mdct_overlap: [[0.0f32; 288]; 2],
            qmf_state: [0.0f32; 960],
            reserv: 0i32,
            free_format_bytes: 0,
            header: [0u8; 4],
            reserv_buf: [0u8; 511],
        },
        pcm: [0 as Sample; MAX_SAMPLES_PER_FRAME],
    }
}

On latest minimp3 there's a new field in mp3dec_frame_info_t being frame_offset so that needs to be set too, but only the first header byte seems to be assumed initialized so any value would probably work. This doesn't seem like it'd fit on the high-speed ram, though... you get 6668+24+(23042) = 11300b for decoder + framedata + pcm 😢 you can* make the frame_info_t not a constant memory location but that only shaves off 24 bytes

[ceigel]

Well, in the end I just can’t put the stack in the fast ram. Minimp3 uses also a lot of stack when called. I haven’t yet figured out how. Here is how I modified the interop library.

https://github.com/ceigel/embedded-mp3

I’ve successfully decoded and played a small mp3 from Rom ~Cristian

On 6. Mar 2020, at 12:40 AM, viri notifications@github.com wrote:

 I wish GitHub notified me about this reply. As far as I know the init function just zeroes a few fields, it should be easy to reimplement as const, although hopefully by now you've had some success!

Edit: Yeah, here it is (header is unsigned char header[4])

void mp3dec_init(mp3dec_t *dec) { dec->header[0] = 0; } Which means you can define a const fn like...

pub const fn new() -> Self { RawDecoder { ffi_frame: ffi::mp3dec_frame_info_t { frame_bytes: 0, channels: 0, hz: 0, layer: 0, bitrate_kbps: 0, }, instance: ffi::mp3dec_t { mdct_overlap: [[0.0f32; 288]; 2], qmf_state: [0.0f32; 960], reserv: 0i32, free_format_bytes: 0, header: [0u8; 4], reserv_buf: [0u8; 511], }, pcm: [0 as Sample; MAX_SAMPLES_PER_FRAME], } } On latest minimp3 there's a new field in mp3dec_frame_info_t being frame_offset so that needs to be set too, but only the first header byte seems to be assumed initialized so any value would probably work. This doesn't seem like it'd fit on the high-speed ram, though... you get 6668+24+(2304*2) = 11300b for decoder + framedata + pcm 😢 you can make the frame_info_t not a constant memory location but that only shaves off 24 bytes

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[lieff]

Minimp3 uses also a lot of stack when called. I haven’t yet figured out how.

It allocates scratch buffer on stack here https://github.com/lieff/minimp3/blob/master/minimp3.h#L1690 because it's needed only temporarily. It can be moved to mp3dec_t structure, or if there only one global decoder simply moved to global variable:

    static mp3dec_scratch_t scratch;

[ceigel]

Probably the best solution is if it is placed in the mp3dec_t structure. I place that in a static variable. Anyway, I probably have space only for one decoder on the microcontroller. The only problem with leaving it on the stack is that there is no consistent way to know how much space it consumes (other than letting the program run and see how deep the stack got). The stack doesn't necessarily need to be placed in the fast RAM. There are many things that could be placed there, including the minimp3 code. Anyway, for your information, I managed to decode a small mp3 and play it from ROM on a STM32F303 microcontroller in Rust.

[0x08088405]

https://github.com/ceigel/embedded-mp3 I’ve successfully decoded and played a small mp3 from Rom

Good work! I like the difference in design, I might do something similar with the result, was never too happy with how next_frame skipped data just so it would return an actual frame.

It allocates scratch buffer on stack here -snip- It can be moved to mp3dec_t structure

Hey, good to see you here! It'd be nice to allow management of the scratch buffer through a parameter or embedding it in the decoder, it's quite big lol

[lieff]

Hi :) Yes, I think about adding MINIMP3_EMBEDDED option. For desktop usage it's no use, since there no need make structure bigger per decoder and also if we use decoders sequentially in one thread, stack which already in L1/L2 cache will be reused while decoder structure increases cache pressure on each additional decoder. So I want to wait if there real use case which needs it before adding option and complexity to code.

[0x08088405]

This is almost a year late but I've merged 0.3, it's a complete rewrite and you can use RawDecoder now which is more or less 1:1 with the minimp3 API.