[soc] libavsequencer [PATCH] Initial implementation of main AVSequencer public API header file
-- Best regards, :-) Basty/CDGS (-:
Sebastian Vater wrote:
+ /** Default volume boost level. 65536 equals to 100% which + means no boost. */ + uint32_t volume_boost; [...] + /** The current volume boost level. 65536 equals to 100% which + means no boost. */ + uint32_t volume_boost; + + /** Left channel volume level. 256 is full volume. */ + uint16_t volume_left; + + /** Right channel volume level. 256 is full volume. */ + uint16_t volume_right; [...] + /** Current volume for this channel which ranges from 0 (muted) + to 255 (full volume). */ + uint8_t volume; + + /** Current stereo panning level for this channel (where 0-127 + indicate left stereo channel panning, -128 is central stereo + panning and -127 to -1 indicate right stereo panning). */ + int8_t panning;
Why all these arbitrary integer scales? Why not floating point or AVRational instead? -Justin
On 07/10/2010 07:00 PM, Justin Ruggles wrote:
Sebastian Vater wrote:
+ /** Default volume boost level. 65536 equals to 100% which + means no boost. */ + uint32_t volume_boost; [...] + /** The current volume boost level. 65536 equals to 100% which + means no boost. */ + uint32_t volume_boost; + + /** Left channel volume level. 256 is full volume. */ + uint16_t volume_left; + + /** Right channel volume level. 256 is full volume. */ + uint16_t volume_right; [...] + /** Current volume for this channel which ranges from 0 (muted) + to 255 (full volume). */ + uint8_t volume; + + /** Current stereo panning level for this channel (where 0-127 + indicate left stereo channel panning, -128 is central stereo + panning and -127 to -1 indicate right stereo panning). */ + int8_t panning;
Why all these arbitrary integer scales? Why not floating point or AVRational instead?
I don't think floating point is a good idea, let's try to keep everything bit-identical across archs if possible. -Vitor
On Sat, Jul 10, 2010 at 07:24:56PM +0200, Vitor Sessak wrote:
On 07/10/2010 07:00 PM, Justin Ruggles wrote:
Sebastian Vater wrote:
+ /** Default volume boost level. 65536 equals to 100% which + means no boost. */ + uint32_t volume_boost; [...] + /** The current volume boost level. 65536 equals to 100% which + means no boost. */ + uint32_t volume_boost; + + /** Left channel volume level. 256 is full volume. */ + uint16_t volume_left; + + /** Right channel volume level. 256 is full volume. */ + uint16_t volume_right; [...] + /** Current volume for this channel which ranges from 0 (muted) + to 255 (full volume). */ + uint8_t volume; + + /** Current stereo panning level for this channel (where 0-127 + indicate left stereo channel panning, -128 is central stereo + panning and -127 to -1 indicate right stereo panning). */ + int8_t panning;
Why all these arbitrary integer scales? Why not floating point or AVRational instead?
I don't think floating point is a good idea, let's try to keep everything bit-identical across archs if possible.
You could still go with something more consistent, like 16.16 fixed-point or AVRational. Particularly uint16_t volume_left; uint8_t volume;
Reimar Döffinger a écrit :
On Sat, Jul 10, 2010 at 07:24:56PM +0200, Vitor Sessak wrote:
On 07/10/2010 07:00 PM, Justin Ruggles wrote:
Sebastian Vater wrote:
+ /** Default volume boost level. 65536 equals to 100% which + means no boost. */ + uint32_t volume_boost; [...] + /** The current volume boost level. 65536 equals to 100% which + means no boost. */ + uint32_t volume_boost; + + /** Left channel volume level. 256 is full volume. */ + uint16_t volume_left; + + /** Right channel volume level. 256 is full volume. */ + uint16_t volume_right; [...] + /** Current volume for this channel which ranges from 0 (muted) + to 255 (full volume). */ + uint8_t volume; + + /** Current stereo panning level for this channel (where 0-127 + indicate left stereo channel panning, -128 is central stereo + panning and -127 to -1 indicate right stereo panning). */ + int8_t panning;
Why all these arbitrary integer scales? Why not floating point or AVRational instead?
I don't think floating point is a good idea, let's try to keep everything bit-identical across archs if possible.
Especially not in data structures which are accessed number of active channels * mixing rate each second...
You could still go with something more consistent, like 16.16 fixed-point or AVRational.
volume_boost is 16.16, I looked at lavu/rational.c it has way to much multiplications making mixing much slower. I don't want to use multiply/divide instructions in the main mixing loop routines.
Particularly uint16_t volume_left; uint8_t volume;
Changing volume_left & volume_right to 16.16 fixed point would be a good idea, though. Normal single channel volume is from 0-255 since that is the volume range in the trackers. -- Best regards, :-) Basty/CDGS (-:
On Sat, Jul 10, 2010 at 07:49:31PM +0200, Sebastian Vater wrote:
Particularly uint16_t volume_left; uint8_t volume;
Changing volume_left & volume_right to 16.16 fixed point would be a good idea, though.
Normal single channel volume is from 0-255 since that is the volume range in the trackers.
You'll have to know it in the end, but I think having different representations and ranges for the volume is going to be very confusing for most people trying to understand the code. Also converting 0-255 range to 16.16 should just be something like (v << 8) + (v >> 7)
Reimar Döffinger a écrit :
On Sat, Jul 10, 2010 at 07:49:31PM +0200, Sebastian Vater wrote:
Particularly uint16_t volume_left; uint8_t volume;
Changing volume_left & volume_right to 16.16 fixed point would be a good idea, though.
Normal single channel volume is from 0-255 since that is the volume range in the trackers.
You'll have to know it in the end, but I think having different representations and ranges for the volume is going to be very confusing for most people trying to understand the code. Also converting 0-255 range to 16.16 should just be something like (v << 8) + (v >> 7)
Hi I have excellent news! libavsequencer now flawlessly integrates into FFmpeg, just check out my latest git. Please do a git pull --rebase, Stefano had problems without using it. Here are the avsequencer.[ch] part of the BSS to review. This version compiles perfectly. -- Best regards, :-) Basty/CDGS (-:
On Sat, Jul 10, 2010 at 06:36:39PM +0200, Sebastian Vater wrote:
--
Best regards, :-) Basty/CDGS (-:
avsequencer.h | 321 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 321 insertions(+) 35215816e65fe3c672d2abb5f39d4c7189e5d26f avsequencer.h.patch diff --git a/libavsequencer/avsequencer.h b/libavsequencer/avsequencer.h new file mode 100644 index 0000000..fcff374 --- /dev/null +++ b/libavsequencer/avsequencer.h @@ -0,0 +1,321 @@ +/* + * AVSequencer main header file which connects to AVFormat and AVCodec + * Copyright (c) 2010 Sebastian Vater <cdgs.basty@googlemail.com> + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef AVSEQUENCER_AVSEQUENCER_H +#define AVSEQUENCER_AVSEQUENCER_H + +#define LIBAVSEQUENCER_VERSION_MAJOR 0 +#define LIBAVSEQUENCER_VERSION_MINOR 0 +#define LIBAVSEQUENCER_VERSION_MICRO 0 + +#define LIBAVSEQUENCER_VERSION_INT AV_VERSION_INT(LIBAVSEQUENCER_VERSION_MAJOR, \ + LIBAVSEQUENCER_VERSION_MINOR, \ + LIBAVSEQUENCER_VERSION_MICRO) +#define LIBAVSEQUENCER_VERSION AV_VERSION(LIBAVSEQUENCER_VERSION_MAJOR, \ + LIBAVSEQUENCER_VERSION_MINOR, \ + LIBAVSEQUENCER_VERSION_MICRO) +#define LIBAVSEQUENCER_BUILD LIBAVSEQUENCER_VERSION_INT + +#define LIBAVSEQUENCER_IDENT "Lavsequencer" AV_STRINGIFY(LIBAVSEQUENCER_VERSION) + +/** + * Returns LIBAVSEQUENCER_VERSION_INT constant. + */ +unsigned avsequencer_version(void); + +/** + * Returns the libavsequencer build-time configuration. + */ +const char *avsequencer_configuration(void); + +/** + * Returns the libavsequencer license. + */ +const char *avsequencer_license(void); + +#include "libavformat/avformat.h" +#include "libavcodec/avcodec.h" +#include "libavsequencer/module.h" +#include "libavsequencer/song.h" +#include "libavsequencer/player.h" +#include "libavutil/tree.h" + +/** + * Mixer context structure which is used to describe certain features + * of registered mixers to the sequencer context. + * New fields can be added to the end with minor version bumps. + * Removal, reordering and changes to existing fields require a major + * version bump. + */ +typedef struct AVSequencerMixerContext {
There will be more than 1 mixer ? [...]
+/** + * Sequencer context structure which is the very root of the + * sequencer. It manages all modules currently in memory, controls + * the playback stuff and declares some customizable lookup tables + * for very strange sound formats. Also all registered mixing engines + * are stored in this structure. + * New fields can be added to the end with minor version bumps. + * Removal, reordering and changes to existing fields require a major + * version bump. + */ +typedef struct AVSequencerContext { + /** Associated decoder packet for this sequencer context. */ + AVPacket *pkt; + + /** Current module used by current playback handler or NULL if + no module is currently being processed. */ + AVSequencerModule *player_module; + + /** Current sub-song used by current playback handler or NULL + if no sub-song is currently being processed. */ + AVSequencerSong *player_song; + + /** Current mixing engine used by current playback handler + or NULL if there is no module and sub-song being processed. */ + AVSequencerMixerData *player_mixer_data; +
+ /** Pointer to sine table for very fast sine calculation. Value + is sin(x)*32767 with one element being one degree. */ + int16_t *sine_lut;
a static or global tables seems cleaner besides we alraedy have sin/cos tables tough they might not be of correct size or type
+ + /** Pointer to linear frequency table for non-Amiga slide modes. + Value is 65536*2^(x/3072). */ + uint16_t *linear_frequency_lut; + + /** Pointer to note calculation frequency table. Value is + 65536*2^(x/12). Please note that the pointer actually points to + the 2nd element. So the base C-4 is [0], -1. B-3 is [-1]. */ + uint32_t *frequency_lut;
is this supposed to be public api? it appears quite internal to me ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin
Michael Niedermayer a écrit :
On Sat, Jul 10, 2010 at 06:36:39PM +0200, Sebastian Vater wrote:
avsequencer.h | 321 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 321 insertions(+) 35215816e65fe3c672d2abb5f39d4c7189e5d26f avsequencer.h.patch diff --git a/libavsequencer/avsequencer.h b/libavsequencer/avsequencer.h new file mode 100644 index 0000000..fcff374 --- /dev/null +++ b/libavsequencer/avsequencer.h @@ -0,0 +1,321 @@ +/* + * AVSequencer main header file which connects to AVFormat and AVCodec + * Copyright (c) 2010 Sebastian Vater <cdgs.basty@googlemail.com> + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef AVSEQUENCER_AVSEQUENCER_H +#define AVSEQUENCER_AVSEQUENCER_H + +#define LIBAVSEQUENCER_VERSION_MAJOR 0 +#define LIBAVSEQUENCER_VERSION_MINOR 0 +#define LIBAVSEQUENCER_VERSION_MICRO 0 + +#define LIBAVSEQUENCER_VERSION_INT AV_VERSION_INT(LIBAVSEQUENCER_VERSION_MAJOR, \ + LIBAVSEQUENCER_VERSION_MINOR, \ + LIBAVSEQUENCER_VERSION_MICRO) +#define LIBAVSEQUENCER_VERSION AV_VERSION(LIBAVSEQUENCER_VERSION_MAJOR, \ + LIBAVSEQUENCER_VERSION_MINOR, \ + LIBAVSEQUENCER_VERSION_MICRO) +#define LIBAVSEQUENCER_BUILD LIBAVSEQUENCER_VERSION_INT + +#define LIBAVSEQUENCER_IDENT "Lavsequencer" AV_STRINGIFY(LIBAVSEQUENCER_VERSION) + +/** + * Returns LIBAVSEQUENCER_VERSION_INT constant. + */ +unsigned avsequencer_version(void); + +/** + * Returns the libavsequencer build-time configuration. + */ +const char *avsequencer_configuration(void); + +/** + * Returns the libavsequencer license. + */ +const char *avsequencer_license(void); + +#include "libavformat/avformat.h" +#include "libavcodec/avcodec.h" +#include "libavsequencer/module.h" +#include "libavsequencer/song.h" +#include "libavsequencer/player.h" +#include "libavutil/tree.h" + +/** + * Mixer context structure which is used to describe certain features + * of registered mixers to the sequencer context. + * New fields can be added to the end with minor version bumps. + * Removal, reordering and changes to existing fields require a major + * version bump. + */ +typedef struct AVSequencerMixerContext {
There will be more than 1 mixer ?
Of course, people are free to contribute more mixers, I will do for my own, two integer only mixers, but if someone wants to do a floating-point mixer, or maybe even a SID-chip mixer is possible to play C64 SID files at a later time? I thought it to be a nice idea to make that extendable as muxers/codecs.
[...]
+/** + * Sequencer context structure which is the very root of the + * sequencer. It manages all modules currently in memory, controls + * the playback stuff and declares some customizable lookup tables + * for very strange sound formats. Also all registered mixing engines + * are stored in this structure. + * New fields can be added to the end with minor version bumps. + * Removal, reordering and changes to existing fields require a major + * version bump. + */ +typedef struct AVSequencerContext { + /** Associated decoder packet for this sequencer context. */ + AVPacket *pkt; + + /** Current module used by current playback handler or NULL if + no module is currently being processed. */ + AVSequencerModule *player_module; + + /** Current sub-song used by current playback handler or NULL + if no sub-song is currently being processed. */ + AVSequencerSong *player_song; + + /** Current mixing engine used by current playback handler + or NULL if there is no module and sub-song being processed. */ + AVSequencerMixerData *player_mixer_data; +
+ /** Pointer to sine table for very fast sine calculation. Value + is sin(x)*32767 with one element being one degree. */ + int16_t *sine_lut;
a static or global tables seems cleaner besides we alraedy have sin/cos tables tough they might not be of correct size or type
See player.c sine_lut, if we have this already we can drop that out.
+ + /** Pointer to linear frequency table for non-Amiga slide modes. + Value is 65536*2^(x/3072). */ + uint16_t *linear_frequency_lut; + + /** Pointer to note calculation frequency table. Value is + 65536*2^(x/12). Please note that the pointer actually points to + the 2nd element. So the base C-4 is [0], -1. B-3 is [-1]. */ + uint32_t *frequency_lut;
is this supposed to be public api? it appears quite internal to me ...
In case some trackers are out there, which use incompatible tables, they can be replaced. I'm not sure right now, if they should be here or handled internally by the lavf (de-)muxers. Anyway these can be set to NULL in that case player.c internal tables are used. They don't have to be filled out by users. Updated avsequencer.h patch included. -- Best regards, :-) Basty/CDGS (-:
On Sun, Jul 11, 2010 at 10:20:26PM +0200, Sebastian Vater wrote:
Michael Niedermayer a écrit :
On Sat, Jul 10, 2010 at 06:36:39PM +0200, Sebastian Vater wrote: [...]
+ + /** Pointer to linear frequency table for non-Amiga slide modes. + Value is 65536*2^(x/3072). */ + uint16_t *linear_frequency_lut; + + /** Pointer to note calculation frequency table. Value is + 65536*2^(x/12). Please note that the pointer actually points to + the 2nd element. So the base C-4 is [0], -1. B-3 is [-1]. */ + uint32_t *frequency_lut;
is this supposed to be public api? it appears quite internal to me ...
In case some trackers are out there, which use incompatible tables, they can be replaced. I'm not sure right now, if they should be here or handled internally by the lavf (de-)muxers. Anyway these can be set to NULL in that case player.c internal tables are used. They don't have to be filled out by users.
dont you think that the public api is a bit big and that a few things feel like they shouldnt be public? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No human being will ever know the Truth, for even if they happen to say it by chance, they would not even known they had done so. -- Xenophanes
On Jul 10, 2010, at 9:36 AM, Sebastian Vater wrote:
--
Best regards, :-) Basty/CDGS (-:
diff --git a/libavsequencer/avsequencer.h b/libavsequencer/avsequencer.h new file mode 100644 index 0000000..fcff374 --- /dev/null +++ b/libavsequencer/avsequencer.h [..] +typedef struct AVSequencerMixerContext { + /** Certain metadata describing this mixer, i.e. who developed + it (artist) and a brief description of the features + (comment). */ + AVMetadata *metadata;
The first member of contexts should be AVClass, so you can use av_log with them.
Alexander Strange a écrit :
On Jul 10, 2010, at 9:36 AM, Sebastian Vater wrote:
--
Best regards, :-) Basty/CDGS (-:
diff --git a/libavsequencer/avsequencer.h b/libavsequencer/avsequencer.h new file mode 100644 index 0000000..fcff374 --- /dev/null +++ b/libavsequencer/avsequencer.h [..] +typedef struct AVSequencerMixerContext { + /** Certain metadata describing this mixer, i.e. who developed + it (artist) and a brief description of the features + (comment). */ + AVMetadata *metadata;
The first member of contexts should be AVClass, so you can use av_log with them.
Fixed. -- Best regards, :-) Basty/CDGS (-:
participants (6)
-
Alexander Strange -
Justin Ruggles -
Michael Niedermayer -
Reimar Döffinger -
Sebastian Vater -
Vitor Sessak