Le septidi 7 fructidor, an CCXXIV, erkki.seppala....@nokia.com a écrit : > From: Erkki Seppälä <erkki.seppala....@nokia.com> > > Added support for passing complex channel layout configuration as side > packet data (AV_PKT_DATA_AUDIO_CHANNEL_LAYOUT, > AV_PKT_DATA_AUDIO_CHANNEL_PREDEFINED_LAYOUT, > AV_PKT_DATA_AUDIO_CHANNEL_LAYOUT_OBJECT_STRUCTURED) to ISO media files > as specified by ISO/IEC 14496-12. > > This information isn't integrated into the existing channel layout > system though,
I think it needs to be.
> which is much more restricted compared to what the
> standard permits.
Certainly, but the bitfield channel layout system is enough for most cases
and well tested. Any new system must work with it, not around it.
> However, the side packet data is structured so that
> it does not require too much ISO base media file format knowledge in
> client code.
>
> This information ends up to an (optional) chnl box in the written file
> in an isom track.
>
> Signed-off-by: Erkki Seppälä <erkki.seppala....@nokia.com>
> Signed-off-by: OZOPlayer <oz...@nokia.com>
> ---
> libavcodec/avcodec.h | 51 ++++++++++++++++++++++++++++++++-
> libavformat/movenc.c | 81
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 130 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 36c85e9..6c64e6a 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1365,6 +1365,35 @@ typedef struct AVTrackReferences {
> /** followed by: int tracks[nb_tracks]; -- tracks this track refers to */
> } AVTrackReferences;
>
> +/** Describes the speaker position of a single audio channel of a single
> track */
Please follow the surrounding style for Doxygen comments: /** and */ on a
separate line, * at the beginning of each line.
> +typedef struct AVAudioTrackChannelPosition {
> + int speaker_position; /** an OutputChannelPosition from ISO/IEC 23001-8
> */
I think most our API users and even developers have better use for 158 Swiss
Francs than feeding the ISO. Please make the API self-sufficient; I suggest
an enum.
> +
> + /** The following are used if speaker_position == 126 */
> + int azimuth;
> + int elevation;
Please document the units and references.
Also, since it is repeated and packed, using a small type would make sense
here if possible.
> +} AVAudioTrackChannelPosition;
> +
> +/** Describes the channel layout (ie. speaker position) of a single audio
> track */
> +typedef struct AVAudioTrackChannelLayout {
> + int nb_positions;
> + /** followed by: AVAudioTrackChannelPosition positions[nb_positions]; */
AVAudioTrackChannelPosition positions[0];
That way, computing the address is taken care of by the compiler without
tricky pointer arithmetic, including alignment requirements.
(We already use 0-sized final arrays once in vaapi_encode.h; since VA API is
for Linux and BSD, it does not probe the crappy proprietary compilers
support it. In that case, using 1 instead of 0 is fine.)
> +} AVAudioTrackChannelLayout;
> +
> +/** Describes the channel layout based on predefined layout of a single track
> + by providing the layout and the list of channels are are omitted */
Could you make that clearer?
> +typedef struct AVAudioTrackChannelPredefinedLayout {
> + int layout; /** ChannelConfiguration from ISO/IEC 23001-8 */
> + int nb_omitted_channels;
> + /** followed by: char omitted_channels[nb_omitted_channels]; - non-zero
> indicates the channel is omitted */
Is there a reasonable upper bound to nb_omitted_channels?
Also, I think the naming is inconsistent: if nb_omitted_channels = 3 and
omitted_channels[] = { 1, 0, 1 }, there are only two omitted channels, not
three.
> +} AVAudioTrackChannelPredefinedLayout;
> +
> +/** Describes the channel layout to be object-sturctued with given
> + number of objects */
Please make that clearer.
> +typedef struct AVAudioTrackChannelLayoutObjectStructured {
> + int object_count;
> +} AVAudioTrackChannelLayoutObjectStructured;
> +
> enum AVPacketSideDataType {
> AV_PKT_DATA_PALETTE,
>
> @@ -1556,7 +1585,27 @@ enum AVPacketSideDataType {
> * Configured the timed metadata parameters, such as the uri and
> * meta data configuration. The key is of type AVTimedMetadata.
> */
> - AV_PKT_DATA_TIMED_METADATA_INFO
> + AV_PKT_DATA_TIMED_METADATA_INFO,
> +
> + /**
> + * Channel layout, describing the position of spakers for the
> + * channels of a track, following the structure
> + * AVAudioTrackChannelLayout.
> + */
> + AV_PKT_DATA_AUDIO_CHANNEL_LAYOUT,
I think it would be a good idea if the name of the side data matches the
name of the structure as much as possible: Track -> _TRACK_. Ditto for the
others.
Would it make sense to have a single side data type and an integer field at
the beginning announcing the type of the rest:
typedef struct AVComplexChannelLayout {
enum AVComplexChannelLayoutType type;
union {
uint64_t bitmask;
AVAudioTrackChannelLayout complete;
AVAudioTrackChannelPredefinedLayout predefined;
AVAudioTrackChannelLayoutObjectStructured object;
} layout;
};
> +
> + /**
> + * Predefined channel layout, describing the position of spakers
> + * for the channels of a track, following the structure
> + * AVAudioTrackChannelPredefinedLayout.
> + */
> + AV_PKT_DATA_AUDIO_CHANNEL_PREDEFINED_LAYOUT,
> +
> + /**
> + * The channel layout is object structured with the number of objects in
> + * AVAudioTrackChannelLayoutObjectStructured
> + */
> + AV_PKT_DATA_AUDIO_CHANNEL_LAYOUT_OBJECT_STRUCTURED
Missing final comma (ditto in the patch adding
AV_PKT_DATA_TIMED_METADATA_INFO; blame to Neil for not adding it to
AV_PKT_DATA_MASTERING_DISPLAY_METADATA).
> };
>
> #define AV_PKT_DATA_QUALITY_FACTOR AV_PKT_DATA_QUALITY_STATS //DEPRECATED
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index ff4bf85..9606918 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
I can not comment on that file.
Regards,
--
Nicolas George
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
