Hi Siarhei,
> > > Appears that bitstream packing is also one of the most CPU hungry
> > > operations in SBC encoder, which did not get proper attention yet.
> > > Could somebody review this patch and provide feedback/comments?
> >
> > again thanks for working on this. I am actually willing to include your
> > patches quickly in one of the next releases to get more wider testing
> > audience.
>
> Thanks. Are the contributors of the last SBC-related modifications reachable
> nowadays? They could probably help with the review of our patches.
they should be at least all subscribed to this mailing list now. Since I
will actually close the other ones in two weeks.
> SBC code still can be improved in many ways from the performance point of
> view. The current implementation follows SBC specification pretty much
> literally. But rearranging the code a bit and getting rid of multidimentional
> arrays can provide identical output, but work noticeably faster. Also SBC
> can benefit from ARM assembly optimizations (for ARM11 and Cortex-A8),
> so these could be applied a bit later once all the high level stuff is in
> place.
>
> Current implementation only needs to be checked to ensure that it is correct
> and fully adheres to the specification before applying large scale
> optimizations.
I agree. So lets get a fully compliant Linux version first and then we
go from there.
> > One comment from my side is that this should work with little-endian and
> > big-endian as input streams. Please keep that in mind.
>
> Yes, it should work fine on both big and little endian systems, though I did
> not test it on big-endian hardware myself. Actually SBC bitstream packer
> favors big-endian systems, because the following part...
>
> + if (bits_count >= 16) {\
> + bits_count -= 8;\
> + *data_ptr++ = (uint8_t)(bits_cache >> bits_count);\
> + bits_count -= 8;\
> + *data_ptr++ = (uint8_t)(bits_cache >> bits_count);\
> + }\
>
> ...could be implemented as something like this for big-endian systems
> (provided that we additionally take care of alignment and convert 'data_ptr'
> into a pointer to uint16_t):
>
> + if (bits_count >= 16) {\
> + bits_count -= 16;\
> + *data_ptr++ = (uint16_t)(bits_cache >> bits_count);\
> + }\
>
> But for little endian-systems and also to ensure endian neutrality, data
> writes are done one byte at a time.
Lets keep doing it byte by byte for now. However keep in mind that the
input stream can also be in big endian even it is runs on a little
machine. And vice-versa.
> And Just an additional comment about portability, there are also systems where
> uint8_t data type is not supported (and CHAR_BIT is different from 8). For
> example, there is a port SBC encoder from bluez to C55x DSP [1]. I'm in no
> way involved in this project, but they could probably want to submit some
> changes to mainline SBC code to make it usable on such systems with a bit
> less tweaks.
We are doing Linux first and then worry about other systems.
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
