[FFmpeg-devel] [PATCH v2 1/5] avutil/mpegts_audio_desc_metadata: add helper function for AC3 descriptor 0x6a

[lance.lmwang at gmail.com]

On Wed, Aug 05, 2020 at 09:55:52AM +0200, Marton Balint wrote:
> 
> 
> On Thu, 30 Jul 2020, lance.lmwang at gmail.com wrote:
> 
> > From: Limin Wang <lance.lmwang at gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
> > ---
> > libavutil/Makefile                     |  2 ++
> > libavutil/mpegts_audio_desc_metadata.c | 33 ++++++++++++++++++++
> > libavutil/mpegts_audio_desc_metadata.h | 57 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 92 insertions(+)
> > create mode 100644 libavutil/mpegts_audio_desc_metadata.c
> > create mode 100644 libavutil/mpegts_audio_desc_metadata.h
> > 
> > diff --git a/libavutil/Makefile b/libavutil/Makefile
> > index 9b08372..4b4aa68 100644
> > --- a/libavutil/Makefile
> > +++ b/libavutil/Makefile
> > @@ -57,6 +57,7 @@ HEADERS = adler32.h                                                     \
> >           md5.h                                                         \
> >           mem.h                                                         \
> >           motion_vector.h                                               \
> > +          mpegts_audio_desc_metadata.h                                  \
> >           murmur3.h                                                     \
> >           opt.h                                                         \
> >           parseutils.h                                                  \
> > @@ -140,6 +141,7 @@ OBJS = adler32.o                                                        \
> >        mastering_display_metadata.o                                     \
> >        md5.o                                                            \
> >        mem.o                                                            \
> > +       mpegts_audio_desc_metadata.o                                     \
> >        murmur3.o                                                        \
> >        opt.o                                                            \
> >        parseutils.o                                                     \
> > diff --git a/libavutil/mpegts_audio_desc_metadata.c b/libavutil/mpegts_audio_desc_metadata.c
> > new file mode 100644
> > index 0000000..14d9100
> > --- /dev/null
> > +++ b/libavutil/mpegts_audio_desc_metadata.c
> > @@ -0,0 +1,33 @@
> > +/*
> > + * Copyright (c) 2020 Limin Wang <lance.lmwang at gmail.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
> > + */
> > +#include "mpegts_audio_desc_metadata.h"
> > +#include "mem.h"
> > +
> > +AVDescriptor6A *av_desc6a_alloc(size_t *size)
> 
> The naming is not OK, as others have already pointed out, because 6A is not
> a good name.
> 
> AVMPEGTSAC3Descriptor or something similar would be better.
> 
> Same goes for the side data type, e.g. AV_PKT_DATA_MPEGTS_AC3_DESC
For AC-3 have two different descriptor for DVB and ATSC standard, so if use
AC-3, DVB or ATSC is necessary also. Also for E-AC3. So I feel they are
too long for the structure and data type. Below is list for compare:

AVMPEGTSDVBAC3Descriptor 
AVMPEGTSDVBEAC3Descriptor 
AVMPEGTSATSCAC3Descriptor 
AVMPEGTSATSCEAC3Descriptor 

vs

AVDescriptor6A
AVDescriptor7A
AVDescriptor81
AVDescriptorCC

AV_PKT_DATA_MPEGTS_DVB_AC3_DESC
AV_PKT_DATA_MPEGTS_DVB_EAC3_DESC
AV_PKT_DATA_MPEGTS_ATSC_AC3_DESC
AV_PKT_DATA_MPEGTS_ATSC_EAC3_DESC

vs

AV_PKT_DATA_MPEGTS_DESC_6A
AV_PKT_DATA_MPEGTS_DESC_7A
AV_PKT_DATA_MPEGTS_DESC_81
AV_PKT_DATA_MPEGTS_DESC_CC

If you prefer to use the long name or have better suggestion, please comments.

Also, I'm not sure wheter it's good solution to add them as side data, how to
process more descriptor like DTS etc?

> 
> > +{
> > +    AVDescriptor6A *desc6a = (AVDescriptor6A*)av_mallocz(sizeof(*desc6a));
> > +
> > +    if (!desc6a)
> > +        return NULL;
> > +    if (size)
> > +        *size = sizeof(*desc6a);
> > +    return desc6a;
> > +
> > +}
> 
> The same kind of alloc function is used for other kinds of side data. Maybe
> it would make sense to figure out some #define magic or something to not
> duplicate this kind of code, but somehow keep type safety?
> 
> E.g. a generic uint8_t *av_alloc_side_data_type(type, *size) and then using
> #defines:
> 
> #define av_alloc_mpegts_ac3_descriptor(size) \
>     (AVMPEGTSAC3Descriptor *)av_alloc_side_data_type(AV_PKT_DATA_MPEGTS_AC3_DESC, size)
> 
> Just an idea...
> 
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-- 
Thanks,
Limin Wang