[FFmpeg-devel] [PATCH 16/21] libavcodec/avcodec, libavformat/movenc: support embedding channel layout to stream side data

[Nicolas George]

Le septidi 7 fructidor, an CCXXIV, erkki.seppala.ext at nokia.com a écrit :
> From: Erkki Seppälä <erkki.seppala.ext at 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.ext at nokia.com>
> Signed-off-by: OZOPlayer <OZOPL at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160823/4e040cc5/attachment.sig>