[FFmpeg-devel] [PATCH] avformat/mxfenc: AVC Intra support

[Tomas Härdin]

On Mon, 2014-10-27 at 17:21 +0000, Thomas Mundt wrote:
> Hi, I´ve seen that there has been approach last month to implement AVC Intra mxf muxing. I tested the patches, but it didn´t work with any of my samples. Since there also has been discussions about the gpl restriction, I rewrote the patch. I had some basics, because I had written a working patch for myself some time ago, which was more of a hack and only worked with AVCI100 1080i50.
> I hope this could be licenced to lgpl, because I got all labels from libmxf and libbmx and only used code snippets from avcodec/h264_parser.c
> To keep h264 parsing simple and fast, I used the framesize for selecting the right Panasonic codec label. The framesize is fixed for Panasonic AVC Intra.
> 
> This patch only supports AVCI50/100. But in all flavours, i.e. with no SPS/PPS in header.
> 
> http://pastebin.com/v7gF1vDq
> 
> Thomas

Could you rewrite it so you don't mix functional changes with
indentation changes? See mxf_write_mpegvideo_desc()
> 
> +    switch (pkt->size + extrasize) {
> +    case 116736: // AVCI50 720p60
> +        sc->codec_ul = &mxf_h264_codec_uls[5];
> +        break;
> +    case 140800: // AVCI50 720p50
> +        sc->codec_ul = &mxf_h264_codec_uls[6];
> +        break;

The magic values here stink. You should stick them next to
mxf_h264_codec_uls, perhaps using a struct like so:

  static const struct {
      UID uid;
      int packet_size;
      int profile;
      uint8_t interlaced;
  } mxf_h264_codec_uls[] = {
      {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x20,0x01 }, 0,      110, 0}, // AVC Intra 50 High 10
      {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x01 }, 232960, 0,   1}, // AVC Intra 50 1080i60
      //etc etc
  };
  static const int mxf_h264_num_codec_uls = sizeof(mxf_h264_codec_uls)/sizeof(mxf_h264_codec_uls[0]);

Then use a little for loop in mxf_parse_h264_frame() to find the
matching entry.

> +            if (desc)
> +                sc->component_depth = desc->comp[0].depth_minus1 + 1;

Seems unrelated?

In general I didn't check how similar this patch is to the GPL'd
version, so I'm going to trust that this doesn't share anything (except
the ULs, which come from the standards).

/Tomas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141027/1d2e9322/attachment.asc>