[FFmpeg-devel,1/3] avformat/mxfenc: Replace more literal magic numbers by enum values.

Submitted by Michael Niedermayer on Sept. 10, 2017, 8:10 p.m.

Details

Message ID 20170910201045.8355-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Sept. 10, 2017, 8:10 p.m.
This also moves the enum table up as it is needed earlier

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mxfenc.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

Comments

Tomas Härdin Sept. 10, 2017, 8:28 p.m.
On Sun, 2017-09-10 at 22:10 +0200, Michael Niedermayer wrote:
>  enum {
>      INDEX_MPEG2 = 0,
>      INDEX_AES3,
> @@ -159,6 +139,26 @@ enum {
>      INDEX_H264,
>  };
>  
> +static const struct {
> +    enum AVCodecID id;
> +    int index;
> +} mxf_essence_mappings[] = {
> +    { AV_CODEC_ID_MPEG2VIDEO, INDEX_MPEG2 },
> +    { AV_CODEC_ID_PCM_S24LE,  INDEX_AES3 },
> +    { AV_CODEC_ID_PCM_S16LE,  INDEX_AES3 },
> +    { AV_CODEC_ID_DVVIDEO,    INDEX_DV },
> +    { AV_CODEC_ID_DNXHD,      INDEX_DNXHD_1080p_10bit_HIGH },
> +    { AV_CODEC_ID_JPEG2000,   INDEX_JPEG2000 },
> +    { AV_CODEC_ID_H264,       INDEX_H264 },
> +    { AV_CODEC_ID_NONE }
> +};

This is tangentally relevant perhaps, but that INDEX_ enum should
really be type. Something like ULIndex and a comment with reference to
relevant spec section would be nice

/Tomas
Michael Niedermayer Sept. 12, 2017, 4:36 p.m.
On Sun, Sep 10, 2017 at 10:28:05PM +0200, Tomas Härdin wrote:
> On Sun, 2017-09-10 at 22:10 +0200, Michael Niedermayer wrote:
> >  enum {
> >      INDEX_MPEG2 = 0,
> >      INDEX_AES3,
> > @@ -159,6 +139,26 @@ enum {
> >      INDEX_H264,
> >  };
> >  
> > +static const struct {
> > +    enum AVCodecID id;
> > +    int index;
> > +} mxf_essence_mappings[] = {
> > +    { AV_CODEC_ID_MPEG2VIDEO, INDEX_MPEG2 },
> > +    { AV_CODEC_ID_PCM_S24LE,  INDEX_AES3 },
> > +    { AV_CODEC_ID_PCM_S16LE,  INDEX_AES3 },
> > +    { AV_CODEC_ID_DVVIDEO,    INDEX_DV },
> > +    { AV_CODEC_ID_DNXHD,      INDEX_DNXHD_1080p_10bit_HIGH },
> > +    { AV_CODEC_ID_JPEG2000,   INDEX_JPEG2000 },
> > +    { AV_CODEC_ID_H264,       INDEX_H264 },
> > +    { AV_CODEC_ID_NONE }
> > +};
> 
> This is tangentally relevant perhaps, but that INDEX_ enum should
> really be type. Something like ULIndex and a comment with reference to
> relevant spec section would be nice

yes, ill change that and ill apply the patchest

thx

[...]
Michael Niedermayer Sept. 12, 2017, 4:45 p.m.
On Sun, Sep 10, 2017 at 10:28:05PM +0200, Tomas Härdin wrote:
> On Sun, 2017-09-10 at 22:10 +0200, Michael Niedermayer wrote:
> >  enum {
> >      INDEX_MPEG2 = 0,
> >      INDEX_AES3,
> > @@ -159,6 +139,26 @@ enum {
> >      INDEX_H264,
> >  };
> >  
> > +static const struct {
> > +    enum AVCodecID id;
> > +    int index;
> > +} mxf_essence_mappings[] = {
> > +    { AV_CODEC_ID_MPEG2VIDEO, INDEX_MPEG2 },
> > +    { AV_CODEC_ID_PCM_S24LE,  INDEX_AES3 },
> > +    { AV_CODEC_ID_PCM_S16LE,  INDEX_AES3 },
> > +    { AV_CODEC_ID_DVVIDEO,    INDEX_DV },
> > +    { AV_CODEC_ID_DNXHD,      INDEX_DNXHD_1080p_10bit_HIGH },
> > +    { AV_CODEC_ID_JPEG2000,   INDEX_JPEG2000 },
> > +    { AV_CODEC_ID_H264,       INDEX_H264 },
> > +    { AV_CODEC_ID_NONE }
> > +};
> 
> This is tangentally relevant perhaps, but that INDEX_ enum should
> really be type. Something like ULIndex and a comment with reference to
> relevant spec section would be nice

About referencing a spec. The index is never written its just used in
our implementation as a way to keep track of the "type" we deal with
so these can actually be reordered / tere values changed with
no change to any output case as long as all tables which imply
this order are also updated.
So i dont think theres a spec that can be referenced

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index cc9ec8c93a..11ccc8c6ec 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -100,26 +100,6 @@  typedef struct MXFContainerEssenceEntry {
     void (*write_desc)(AVFormatContext *, AVStream *);
 } MXFContainerEssenceEntry;
 
-static const struct {
-    enum AVCodecID id;
-    int index;
-} mxf_essence_mappings[] = {
-    { AV_CODEC_ID_MPEG2VIDEO, 0 },
-    { AV_CODEC_ID_PCM_S24LE,  1 },
-    { AV_CODEC_ID_PCM_S16LE,  1 },
-    { AV_CODEC_ID_DVVIDEO,   15 },
-    { AV_CODEC_ID_DNXHD,     24 },
-    { AV_CODEC_ID_JPEG2000,  34 },
-    { AV_CODEC_ID_H264,      35 },
-    { AV_CODEC_ID_NONE }
-};
-
-static void mxf_write_wav_desc(AVFormatContext *s, AVStream *st);
-static void mxf_write_aes3_desc(AVFormatContext *s, AVStream *st);
-static void mxf_write_mpegvideo_desc(AVFormatContext *s, AVStream *st);
-static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st);
-static void mxf_write_generic_sound_desc(AVFormatContext *s, AVStream *st);
-
 enum {
     INDEX_MPEG2 = 0,
     INDEX_AES3,
@@ -159,6 +139,26 @@  enum {
     INDEX_H264,
 };
 
+static const struct {
+    enum AVCodecID id;
+    int index;
+} mxf_essence_mappings[] = {
+    { AV_CODEC_ID_MPEG2VIDEO, INDEX_MPEG2 },
+    { AV_CODEC_ID_PCM_S24LE,  INDEX_AES3 },
+    { AV_CODEC_ID_PCM_S16LE,  INDEX_AES3 },
+    { AV_CODEC_ID_DVVIDEO,    INDEX_DV },
+    { AV_CODEC_ID_DNXHD,      INDEX_DNXHD_1080p_10bit_HIGH },
+    { AV_CODEC_ID_JPEG2000,   INDEX_JPEG2000 },
+    { AV_CODEC_ID_H264,       INDEX_H264 },
+    { AV_CODEC_ID_NONE }
+};
+
+static void mxf_write_wav_desc(AVFormatContext *s, AVStream *st);
+static void mxf_write_aes3_desc(AVFormatContext *s, AVStream *st);
+static void mxf_write_mpegvideo_desc(AVFormatContext *s, AVStream *st);
+static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st);
+static void mxf_write_generic_sound_desc(AVFormatContext *s, AVStream *st);
+
 static const MXFContainerEssenceEntry mxf_essence_container_uls[] = {
     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x02,0x0D,0x01,0x03,0x01,0x02,0x04,0x60,0x01 },
       { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x15,0x01,0x05,0x00 },