Re: [FFmpeg-devel] [PATCH v2] added ULs for demuxing avid media composer mxf files

[Tomas Härdin]

On Tue, 2014-08-12 at 14:58 -0700, Mark Reid wrote:
 UL values copied from FMbc version of mxf.c
 Fixes Ticket#1554, Ticket#3100 and Ticket#3450
 ---
  libavformat/mxf.c | 3 +++
  1 file changed, 3 insertions(+)
 
 diff --git a/libavformat/mxf.c b/libavformat/mxf.c
 index d20ed94..8ebfdf2 100644
 --- a/libavformat/mxf.c
 +++ b/libavformat/mxf.c
 @@ -28,6 +28,8 @@
  const MXFCodecUL ff_mxf_data_definition_uls[] = {
  { { 
 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x01,0x03,0x02,0x02,0x01,0x00,0x00,0x00
  }, 13, AVMEDIA_TYPE_VIDEO },
  { { 
 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x01,0x03,0x02,0x02,0x02,0x00,0x00,0x00
  }, 13, AVMEDIA_TYPE_AUDIO },
 +{ { 
 0x80,0x7D,0x00,0x60,0x08,0x14,0x3E,0x6F,0x6F,0x3C,0x8C,0xE1,0x6C,0xEF,0x11,0xD2
  }, 16, AVMEDIA_TYPE_VIDEO }, /* Avid Media Composer MXF */
 +{ { 
 0x80,0x7D,0x00,0x60,0x08,0x14,0x3E,0x6F,0x78,0xE1,0xEB,0xE1,0x6C,0xEF,0x11,0xD2
  }, 16, AVMEDIA_TYPE_AUDIO }, /* Avid Media Composer MXF */
  { { 
 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00
  },  0,  AVMEDIA_TYPE_DATA },
  };
  
 @@ -44,6 +46,7 @@ const MXFCodecUL ff_mxf_codec_uls[] = {
  { { 
 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x01,0x01,0x02,0x01,0x00
  }, 15,   AV_CODEC_ID_RAWVIDEO }, /* Uncompressed 422 8-bit */
  { { 
 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x02,0x71,0x00,0x00,0x00
  }, 13,  AV_CODEC_ID_DNXHD }, /* SMPTE VC-3/DNxHD */
  { { 
 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x02,0x03,0x02,0x00,0x00
  }, 14,  AV_CODEC_ID_DNXHD }, /* SMPTE VC-3/DNxHD */
 +{ { 
 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0E,0x04,0x02,0x01,0x02,0x04,0x01,0x00
  }, 16,  AV_CODEC_ID_DNXHD }, /* SMPTE VC-3/DNxHD Avid Media Composer MXF 
 */
  { { 
 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x02,0x01,0x32,0x00,0x00
  }, 14,   AV_CODEC_ID_H264 }, /* H.264/MPEG-4 AVC Intra */
  { { 
 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x02,0x01,0x31,0x11,0x01
  }, 14,   AV_CODEC_ID_H264 }, /* H.264/MPEG-4 AVC SPS/PPS in-band */
  { { 
 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x01,0x01,0x02,0x02,0x01
  }, 16,   AV_CODEC_ID_V210 }, /* V210 */

Looks OK. Checked the codec UL, seems correct.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]Do not ask for mxf samples with unknown field dominance

[Tomas Härdin]

On Tue, 2014-08-19 at 22:30 +0200, Michael Niedermayer wrote:
 On Tue, Aug 19, 2014 at 01:30:24AM +0200, Carl Eugen Hoyos wrote:
  Hi!
  
  Attached patch removes a request for samples of which we already
  have several that all work fine.
 
 field_dominance can have 256 different values, do we have samples
 for all ? do they even exist ?

As far as I recall there are only two (1, 2) and possibly unknown (0).

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] MXF : default fied dominance is TFF

[Tomas Härdin]

On Tue, 2014-09-09 at 12:37 +0200, Michael Niedermayer wrote:
 On Mon, Sep 08, 2014 at 12:17:14PM +, Gaullier Nicolas wrote:
  I did not found an easy way to set up initialization values to properly 
  handle defaults but I am not a highly skilled developer, and maybe someone 
  will find how to implement this more elegantly.
  They are also many other properties in mxf that are only optional, for 
  example component depth and horizontal/vertical subsampling factors that 
  are actually parsed, but as far from now it does not seem sufficiently 
  useful to distinguish between the initialization value '0' and 'not 
  present'.
  In my opinion, in the solely case of the field dominance, when it is 
  found/set to '0', it seems interesting to fail/raise a warning at least, 
  but it is somewhat particular and should not involve a big code refactoring 
  to handle this.
 
 field_dominance could be initilaized to -1 (or some named identifer
 that is -1)
 and a case -1 be added to the switch()

Initializing to the default value (1 = MXF_TFF) makes more sense. For
any illegal value the demuxer should probably complain so the user can
complain at whoever wrote the bad muxer they're using.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]Do not ask for mxf samples with unknown field dominance

[Tomas Härdin]

On Tue, 2014-09-16 at 09:03 +0100, tim nicholson wrote:
 On 10/09/14 21:45, Tomas Härdin wrote:
  On Tue, 2014-08-19 at 22:30 +0200, Michael Niedermayer wrote:
  On Tue, Aug 19, 2014 at 01:30:24AM +0200, Carl Eugen Hoyos wrote:
  Hi!
 
  Attached patch removes a request for samples of which we already
  have several that all work fine.
 
  field_dominance can have 256 different values, do we have samples
  for all ? do they even exist ?
  
 
 Whilst the variable may be able to contain 256 different values in
 reality there are only two possible states f1 first or f2 first.
 
  As far as I recall there are only two (1, 2) and possibly unknown (0).
  
 
 Is there a distinction between unknown and progressive? I don't have
 the numbers to hand.

Unknown means unknown. If it's an optional field it probably means the
muxer that made the file is crap, which is not uncommon.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] avformat/mxfenc: H.264 Intra support

[Tomas Härdin]

On Sat, 2014-09-13 at 12:36 +0200, Michael Niedermayer wrote:
 From: Baptiste Coudurier baptiste.coudur...@gmail.com
 
 Ported by michael from ffmbc to ffmpeg
 the code is under CONFIG_GPL as ffmbc is GPL
 
 Signed-off-by: Michael Niedermayer michae...@gmx.at
 ---
  libavformat/mxfenc.c |  143 
 ++
  1 file changed, 143 insertions(+)

 +static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
 +AVPacket *pkt, MXFIndexEntry *e)
 +{
 +MXFStreamContext *sc = st-priv_data;
 +H264Context h;
 +const uint8_t *buf = pkt-data;
 +const uint8_t *buf_end = pkt-data + pkt-size;
 +uint32_t state = -1;
 +AVRational sar = {1, 1};
 +
 +if (pkt-size  5 || AV_RB32(pkt-data) != 0x001) {
 +av_log(s, AV_LOG_ERROR, h264 bitstream malformated, 
 +   no startcode found, use -vbsf h264_mp4toannexb\n);
 +return 0;
 +}
 +
 +memset(h, 0, sizeof(h));
 +h.avctx = st-codec;
 +h.thread_context[0] = h;
 +h.prev_frame_num = -1;
 +
 +for (;;) {
 +int src_length, dst_length, consumed;
 +const uint8_t *ptr;
 +
 +buf = avpriv_find_start_code(buf, buf_end, state);
 +if (buf = buf_end)
 +break;
 +--buf;
 +src_length = buf_end - buf;
 +switch (state  0x1f) {
 +case NAL_SLICE:
 +case NAL_IDR_SLICE:
 +// Do not walk the whole buffer just to decode slice header
 +if (src_length  20)
 +src_length = 20;
 +break;
 +}
 +
 +ptr = ff_h264_decode_nal(h, buf, dst_length, consumed, 
 src_length);
 +if (!ptr || dst_length  0)
 +break;
 +
 +init_get_bits(h.gb, ptr, 8*dst_length);
 +//av_log(avctx, AV_LOG_DEBUG, nal_unit_type:%d\n, h.nal_unit_type);
 +switch (h.nal_unit_type) {
 +case NAL_SPS:
 +ff_h264_decode_seq_parameter_set(h);
 +if (h.sps.sar.num  0  h.sps.sar.den  0) {
 +sar.num = st-codec-width * h.sps.sar.num;
 +sar.den = st-codec-height * h.sps.sar.den;
 +}
 +sc-aspect_ratio.num = st-codec-width * sar.num;
 +sc-aspect_ratio.den = st-codec-height * sar.den;
 +av_reduce(sc-aspect_ratio.num, sc-aspect_ratio.den,
 +  sar.num, sar.den, 1024*1024);
 +
 +sc-interlaced = !h.sps.frame_mbs_only_flag;
 +sc-component_depth = h.sps.bit_depth_luma;
 +
 +sc-codec_ul = mxf_get_h264_codec_ul(st-codec, h.sps);
 +e-flags |= 0x40;
 +break;
 +case NAL_PPS:
 +ff_h264_decode_picture_parameter_set(h, h.gb.size_in_bits);
 +if (h.sps.timing_info_present_flag) {
 +if (st-codec-time_base.num != h.sps.num_units_in_tick ||
 +st-codec-time_base.den*2 != h.sps.time_scale) {
 +av_log(s, AV_LOG_ERROR, framerate does not match 
 bitstream values: %d/%d != %d/%d\n,
 +   h.sps.num_units_in_tick, h.sps.time_scale, 
 st-codec-time_base.num, st-codec-time_base.den*2);
 +return 0;
 +}
 +}
 +if (e-flags  0x40) // sequence header present
 +e-flags |= 0x80; // random access
 +break;
 +case NAL_IDR_SLICE:
 +break;
 +case NAL_SLICE:
 +av_log(s, AV_LOG_ERROR, mxf muxer only supports AVC Intra 
 currently\n);
 +return 0;
 +}
 +}
 +
 +return !!sc-codec_ul;
 +}

Seems awfully specific code to have in a muxer. Can this be reused
elsewhere? Don't let that stop you though.

Overall I'm OK with this patch being GPL. It's not optimal, but I
compile with libx264 for my own purposes anyway and thus have GPL
enabled. If someone wants it LGPL bad enough then they're free to
rewrite this of course. Perhaps as a part of making the functionality of
the above function reusable elsewhere? (I think MOV might support
AVC-Intra)

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: read reel_name and source timecode from physical source package

[Tomas Härdin]

On Thu, 2014-09-25 at 16:13 -0700, Mark Reid wrote:
 ---
  libavformat/mxfdec.c | 118 
 ++-
  1 file changed, 97 insertions(+), 21 deletions(-)
 
 diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
 index 7a4633f..3a1889f 100644
 --- a/libavformat/mxfdec.c
 +++ b/libavformat/mxfdec.c
 @@ -188,6 +188,7 @@ typedef struct {
  int tracks_count;
  MXFDescriptor *descriptor; /* only one */
  UID descriptor_ref;
 +char *name;
  } MXFPackage;
  
  typedef struct {
 @@ -731,6 +732,27 @@ static int mxf_read_sequence(void *arg, AVIOContext *pb, 
 int tag, int size, UID
  return 0;
  }
  
 +static int mxf_read_utf16_string(AVIOContext *pb, int size, char** str)
 +{
 +int ret;
 +size_t buf_size;
 +
 +if (size  0)
 +return AVERROR(EINVAL);
 +
 +buf_size = size + size / 2 + 1;

This should be a function, like ff_utf16_buflen() or something. I see
that this hunk is just moving the function though, so don't let that
hold this patch up.

 +*str = av_malloc(buf_size);
 +if (!*str)
 +return AVERROR(ENOMEM);
 +
 +if ((ret = avio_get_str16be(pb, size, *str, buf_size))  0) {
 +av_freep(str);
 +return ret;
 +}
 +
 +return ret;
 +}


 +static int mxf_parse_physical_source_package(MXFContext *mxf, MXFTrack 
 *source_track, AVStream *st)
 +{
  [...]
 +
 +for (k = 0; k  
 physical_track-sequence-structural_components_count; k++) {
 +component = mxf_resolve_strong_ref(mxf, 
 physical_track-sequence-structural_components_refs[k], TimecodeComponent);
 +if (!component)
 +continue;
 +
 +mxf_tc = (MXFTimecodeComponent*)component;
 +flags = mxf_tc-drop_frame == 1 ? AV_TIMECODE_FLAG_DROPFRAME 
 : 0;
 +/* scale sourceclip start_position to match physical track 
 edit rate */
 +start_position = av_rescale_q(sourceclip-start_position, 
 av_inv_q(source_track-edit_rate), av_inv_q(physical_track-edit_rate));

av_rescale()

 +
 +if (av_timecode_init(tc, mxf_tc-rate, flags, 
 start_position + mxf_tc-start_frame, mxf-fc) == 0) {
 +mxf_add_timecode_metadata(st-metadata, timecode, 
 tc);
 +return 0;
 +}
 +}
 +}
 +}
 +
 +return 0;
 +}
 +

Looks OK otherwise. Did you try fuzzing (zzuf) with a few files too?

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mxfenc: add jpeg2000 support

[Tomas Härdin]

On Tue, 2014-09-30 at 15:37 +0200, Benoit Fouet wrote:
 Hi,
 
 this patch adds support for j2k muxing in MXF.
 tested with:
 $ ffmpeg -t 5 -f lavfi -i testsrc -y -c:v libopenjpeg -y out.mxf
 
 Played back in ffplay (linux), vlc (windows), Acrok MXF converter (windows).
 I have no idea against what other players this should be tested.

Do we have a sample + FATE case for this?

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] mxfdec: minor simplification.

[Tomas Härdin]

On Sat, 2014-10-18 at 15:26 +0200, Reimar Döffinger wrote:
 Signed-off-by: Reimar Döffinger reimar.doeffin...@gmx.de
 ---
  libavformat/mxfdec.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
 index 9306cc6..f916d01 100644
 --- a/libavformat/mxfdec.c
 +++ b/libavformat/mxfdec.c
 @@ -2442,7 +2442,7 @@ static int mxf_read_packet_old(AVFormatContext *s, 
 AVPacket *pkt)
  pkt-stream_index = index;
  pkt-pos = klv.offset;
  
 -codec = s-streams[index]-codec;
 +codec = st-codec;
  
  if (codec-codec_type == AVMEDIA_TYPE_VIDEO  next_ofs = 0) {
  /* mxf-current_edit_unit good - see if we have an

OK


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Tomas Härdin]

On Mon, 2014-10-27 at 17:21 +, 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



signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/mxfdec.c: refactored resolving timecode component

[Tomas Härdin]

On Wed, 2014-11-12 at 12:15 -0800, Mark Reid wrote:
 ---
  libavformat/mxfdec.c | 36 +++-
  1 file changed, 23 insertions(+), 13 deletions(-)
 
 diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
 index b533e2a..87f1e51 100644
 --- a/libavformat/mxfdec.c
 +++ b/libavformat/mxfdec.c
 @@ -1424,6 +1424,27 @@ static int mxf_add_timecode_metadata(AVDictionary 
 **pm, const char *key, AVTimec
  return 0;
  }
  
 +static MXFTimecodeComponent* mxf_resolve_timecode_component(MXFContext *mxf, 
 UID *strong_ref)
 +{
 +MXFStructuralComponent *component = NULL;
 +MXFPulldownComponent *pulldown = NULL;
 +
 +component = mxf_resolve_strong_ref(mxf, strong_ref, AnyType);
 +if (!component)
 +return NULL;
 +
 +switch (component-type) {
 +case TimecodeComponent:
 +return (MXFTimecodeComponent*)component;
 +case PulldownComponent: /* timcode component may be located on a 
 pulldown component */
 +pulldown = (MXFPulldownComponent*)component;
 +return mxf_resolve_strong_ref(mxf, pulldown-input_segment_ref, 
 TimecodeComponent);
 +default:
 +break;
 +}
 +return NULL;
 +}
 +
  static int mxf_parse_physical_source_package(MXFContext *mxf, MXFTrack 
 *source_track, AVStream *st)
  {
  MXFPackage *temp_package = NULL;
 @@ -1432,7 +1453,6 @@ static int mxf_parse_physical_source_package(MXFContext 
 *mxf, MXFTrack *source_t
  MXFStructuralComponent *component = NULL;
  MXFStructuralComponent *sourceclip = NULL;
  MXFTimecodeComponent *mxf_tc = NULL;
 -MXFPulldownComponent *mxf_pulldown = NULL;
  int i, j, k;
  AVTimecode tc;
  int flags;
 @@ -1475,19 +1495,9 @@ static int 
 mxf_parse_physical_source_package(MXFContext *mxf, MXFTrack *source_t
  }
  
  for (k = 0; k  
 physical_track-sequence-structural_components_count; k++) {
 -component = mxf_resolve_strong_ref(mxf, 
 physical_track-sequence-structural_components_refs[k], TimecodeComponent);
 -if (!component){
 -/* timcode component may be located on a pulldown 
 component */
 -component = mxf_resolve_strong_ref(mxf, 
 physical_track-sequence-structural_components_refs[k], PulldownComponent);
 -if (!component)
 -continue;
 -mxf_pulldown = (MXFPulldownComponent*)component;
 -component = mxf_resolve_strong_ref(mxf, 
 mxf_pulldown-input_segment_ref, TimecodeComponent);
 -if (!component)
 -continue;
 -}
 +if (!(mxf_tc = mxf_resolve_timecode_component(mxf, 
 physical_track-sequence-structural_components_refs[k])))
 +continue;
  
 -mxf_tc = (MXFTimecodeComponent*)component;
  flags = mxf_tc-drop_frame == 1 ? AV_TIMECODE_FLAG_DROPFRAME 
 : 0;
  /* scale sourceclip start_position to match physical track 
 edit rate */
  start_position = av_rescale_q(sourceclip-start_position,

Looks simple enough. Good initiative :)

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]Read aspect ratio from mxf

[Tomas Härdin]

On Sun, 2014-11-16 at 02:03 +0100, Carl Eugen Hoyos wrote:
 On Saturday 15 November 2014 11:57:00 pm Michael Niedermayer wrote:
  On Sat, Nov 15, 2014 at 02:50:38AM +0100, Carl Eugen Hoyos wrote:
   Hi!
  
   Attached patch fixes ticket #4107 for me.
   An alternative would be to force the sar to 4:3
   if h264 10bit 1440x1080 video has sar 3:4.
 
   +av_dict_set(st-metadata, display_aspect_ratio_num,
   NULL, 0); +av_dict_set(st-metadata,
   display_aspect_ratio_den, NULL, 0); +}
 
  I suggest you add a documented as private/internal
  display_aspect_ratio to AVStream instead of metadata
  also av_reduce can be replaced by av_mul_q which is probably simpler
 
 New patch attached.
 
 Thank you, Carl Eugen

Looks good to me. I recall doing something similar way back that I never
sent to this list, in order to deal with similar issues.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/mxfdec.c: export source package uids and names as metadata

[Tomas Härdin]

On Tue, 2014-11-18 at 16:52 -0800, Mark Reid wrote:
 ---
  libavformat/mxfdec.c | 74 
 +++-
  1 file changed, 39 insertions(+), 35 deletions(-)
 
 diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
 index fa0a2f4..8c13c24 100644
 --- a/libavformat/mxfdec.c
 +++ b/libavformat/mxfdec.c

  static int mxf_timestamp_to_str(uint64_t timestamp, char **str)
  {
  struct tm time = { 0 };
 @@ -1969,7 +1973,7 @@ static const MXFMetadataReadTableEntry 
 mxf_metadata_read_table[] = {
  { { 
 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x30,0x00
  }, mxf_read_identification_metadata },
  { { 
 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x18,0x00
  }, mxf_read_content_storage, 0, AnyType },
  { { 
 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x37,0x00
  }, mxf_read_source_package, sizeof(MXFPackage), SourcePackage },
 -{ { 
 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x36,0x00
  }, mxf_read_material_package, sizeof(MXFPackage), MaterialPackage },
 +{ { 
 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x36,0x00
  }, mxf_read_source_package, sizeof(MXFPackage), MaterialPackage },

Maybe rename mxf_read_source_package to mxf_read_package?
Noticing that both functions are quite similar is a good catch :)
The rest of the patch looks fine.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/mxfdec.c: initial support for EssenceGroups

[Tomas Härdin]

On Tue, 2014-11-25 at 15:14 -0800, Mark Reid wrote:
 ---
  libavformat/mxf.c  |   1 +
  libavformat/mxf.h  |   1 +
  libavformat/mxfdec.c   | 148 
 -
  tests/ref/lavf/mxf |   6 +-
  tests/ref/lavf/mxf_d10 |   2 +-
  5 files changed, 127 insertions(+), 31 deletions(-)
 
 diff --git a/libavformat/mxf.c b/libavformat/mxf.c
 index 4dc54d7..14d143e 100644
 --- a/libavformat/mxf.c
 +++ b/libavformat/mxf.c
 @@ -94,6 +94,7 @@ static const struct {
  {AV_PIX_FMT_RGB565BE,{'R', 5,  'G', 6,  'B', 5 
 }},
  {AV_PIX_FMT_RGBA,{'R', 8,  'G', 8,  'B', 8, 'A', 8 
 }},
  {AV_PIX_FMT_PAL8,{'P', 8   
 }},
 +{AV_PIX_FMT_GRAY8,   {'A', 8   
 }},

There's no pixel format for pure alpha?

  };
  
  static const int num_pixel_layouts = FF_ARRAY_ELEMS(ff_mxf_pixel_layouts);
 diff --git a/libavformat/mxf.h b/libavformat/mxf.h
 index 5b95efa..63b497a 100644
 --- a/libavformat/mxf.h
 +++ b/libavformat/mxf.h
 @@ -35,6 +35,7 @@ enum MXFMetadataSetType {
  TimecodeComponent,
  PulldownComponent,
  Sequence,
 +EssenceGroup,

Add this to the end of the enum? That way mxfenc output doesn't change.

  MultipleDescriptor,
  Descriptor,
  Track,
  [...]
  
 +static int mxf_read_essence_group(void *arg, AVIOContext *pb, int tag, int 
 size, UID uid, int64_t klv_offset)
 +{
 +MXFEssenceGroup *essence_group = arg;
 +switch (tag) {
 +case 0x0202:
 +essence_group-duration = avio_rb64(pb);
 +break;
 +case 0x0501:
 +essence_group-structural_components_count = avio_rb32(pb);
 +essence_group-structural_components_refs = 
 av_calloc(essence_group-structural_components_count, sizeof(UID));
 +if (!essence_group-structural_components_refs)
 +return AVERROR(ENOMEM);
 +avio_skip(pb, 4); /* useless size of objects, always 16 according to 
 specs */
 +avio_read(pb, (uint8_t *)essence_group-structural_components_refs, 
 essence_group-structural_components_count * sizeof(UID));

Is there any risk of this multiplication overflowing? I suspect
av_calloc() will fail if it does, just making sure. Also not critical if
it actually does since avio_read() just ends up reading less.

 +static MXFStructuralComponent* mxf_resolve_essence_group_choice(MXFContext 
 *mxf, MXFEssenceGroup *essence_group)
 +{
 +MXFStructuralComponent *component = NULL;
 +MXFPackage *package = NULL;
 +MXFDescriptor *descriptor = NULL;
 +int i;
 +
 +if (!essence_group || !essence_group-structural_components_count)
 +return NULL;
 +
 +/* essence groups contains multiple representations of the same media,
 +   this return the first components with a valid Descriptor typically 
 index 0 */
 +for (i =0; i  essence_group-structural_components_count; i++){
 +component = mxf_resolve_strong_ref(mxf, 
 essence_group-structural_components_refs[i], SourceClip);
 +if (!component)
 +continue;
 +
 +if (!(package = mxf_resolve_source_package(mxf, 
 component-source_package_uid)))
 +continue;
 +
 +descriptor = mxf_resolve_strong_ref(mxf, package-descriptor_ref, 
 Descriptor);
 +if (descriptor){
 +/* HACK: force the duration of the component to match the 
 duration of the descriptor */
 +if (descriptor-duration != AV_NOPTS_VALUE)
 +component-duration = descriptor-duration;

Not a huge fan of this, but probably doesn't hurt since it's checking
for AV_NOPTS_VALUE.

 +static int mxf_metadataset_init(MXFMetadataSet *ctx, enum MXFMetadataSetType 
 type)
 +{
 +switch (type){
 +case MultipleDescriptor:
 +case Descriptor:
 +((MXFDescriptor*)ctx)-pix_fmt = AV_PIX_FMT_NONE;
 +((MXFDescriptor*)ctx)-duration = AV_NOPTS_VALUE;
 +break;
 +default:
 +break;
 +}
 +return 0;
 +}
 +

Good idea :)

 diff --git a/tests/ref/lavf/mxf b/tests/ref/lavf/mxf
 index 236661c..d237560 100644
 --- a/tests/ref/lavf/mxf
 +++ b/tests/ref/lavf/mxf

Again, probably not needed if you stick EssenceGroup at the end of the
enum.

I like using mxf_resolve_source_package() to reduce the size of
mxf_parse_physical_source_package() and mxf_parse_structural_metadata().

Memory handling looks correct too. Did you double-check with valgrind?

Looks pretty good overall.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/mxfdec.c refactor resolving MultiDescriptor and remove essence group hack

[Tomas Härdin]

On Tue, 2014-12-02 at 16:43 -0800, Mark Reid wrote:
 I think this is a better way to deal with single frame essence data then my
 previous way.
 
 ---
  libavformat/mxfdec.c | 62 
 
  1 file changed, 38 insertions(+), 24 deletions(-)
 
 diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
 index 0c88a8a..6c104b9 100644
 --- a/libavformat/mxfdec.c
 +++ b/libavformat/mxfdec.c
 @@ -1508,6 +1508,32 @@ static MXFPackage* 
 mxf_resolve_source_package(MXFContext *mxf, UID package_uid)
  return NULL;
  }
  
 +static MXFDescriptor* mxf_resolve_multidescriptor(MXFContext *mxf, 
 MXFDescriptor *descriptor, int track_id)
 +{
 +MXFDescriptor *sub_descriptor = NULL;
 +int i;
 +
 +if (!descriptor)
 +return NULL;
 +
 +if (descriptor-type == MultipleDescriptor) {
 +for (i = 0; i  descriptor-sub_descriptors_count; i++) {
 +sub_descriptor = mxf_resolve_strong_ref(mxf, 
 descriptor-sub_descriptors_refs[i], Descriptor);
 +
 +if (!sub_descriptor) {
 +av_log(mxf-fc, AV_LOG_ERROR, could not resolve sub 
 descriptor strong ref\n);
 +continue;
 +}
 +if (sub_descriptor-linked_track_id == track_id) {
 +return sub_descriptor;
 +}
 +}
 +} else if (descriptor-type == Descriptor)
 +return descriptor;
 +
 +return NULL;
 +}
 +
  static MXFStructuralComponent* mxf_resolve_essence_group_choice(MXFContext 
 *mxf, MXFEssenceGroup *essence_group)
  {
  MXFStructuralComponent *component = NULL;
 @@ -1529,12 +1555,8 @@ static MXFStructuralComponent* 
 mxf_resolve_essence_group_choice(MXFContext *mxf,
  continue;
  
  descriptor = mxf_resolve_strong_ref(mxf, package-descriptor_ref, 
 Descriptor);
 -if (descriptor){
 -/* HACK: force the duration of the component to match the 
 duration of the descriptor */
 -if (descriptor-duration != AV_NOPTS_VALUE)
 -component-duration = descriptor-duration;
 +if (descriptor)
  return component;
 -}
  }
  return NULL;
  }
 @@ -1735,7 +1757,17 @@ static int mxf_parse_structural_metadata(MXFContext 
 *mxf)
  }
  st-id = source_track-track_id;
  st-priv_data = source_track;
 -source_track-original_duration = st-duration = component-duration;
 +
 +source_package-descriptor = mxf_resolve_strong_ref(mxf, 
 source_package-descriptor_ref, AnyType);
 +descriptor = mxf_resolve_multidescriptor(mxf, 
 source_package-descriptor, source_track-track_id);
 +
 +/* A SourceClip from a EssenceGroup may only be a single frame of 
 essence data. The clips duration is then how many
 + * frames its suppose to repeat for. Descriptor-duration, if 
 present, contains the real duration of the essence data */
 +if (descriptor  descriptor-duration != AV_NOPTS_VALUE)
 +source_track-original_duration = st-duration = 
 FFMIN(descriptor-duration, component-duration);
 +else
 +source_track-original_duration = st-duration = 
 component-duration;
 +
  if (st-duration == -1)
  st-duration = AV_NOPTS_VALUE;
  st-start_time = component-start_position;
 @@ -1758,24 +1790,6 @@ static int mxf_parse_structural_metadata(MXFContext 
 *mxf)
  codec_ul = mxf_get_codec_ul(ff_mxf_data_definition_uls, 
 source_track-sequence-data_definition_ul);
  st-codec-codec_type = codec_ul-id;
  
 -source_package-descriptor = mxf_resolve_strong_ref(mxf, 
 source_package-descriptor_ref, AnyType);
 -if (source_package-descriptor) {
 -if (source_package-descriptor-type == MultipleDescriptor) {
 -for (j = 0; j  
 source_package-descriptor-sub_descriptors_count; j++) {
 -MXFDescriptor *sub_descriptor = 
 mxf_resolve_strong_ref(mxf, 
 source_package-descriptor-sub_descriptors_refs[j], Descriptor);
 -
 -if (!sub_descriptor) {
 -av_log(mxf-fc, AV_LOG_ERROR, could not resolve sub 
 descriptor strong ref\n);
 -continue;
 -}
 -if (sub_descriptor-linked_track_id == 
 source_track-track_id) {
 -descriptor = sub_descriptor;
 -break;
 -}
 -}
 -} else if (source_package-descriptor-type == Descriptor)
 -descriptor = source_package-descriptor;
 -}
  if (!descriptor) {
  av_log(mxf-fc, AV_LOG_INFO, source track %d: stream %d, no 
 descriptor found\n, source_track-track_id, st-index);
  continue;


Seems alright..

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org

Re: [FFmpeg-devel] [PATCH]Fix leak reading invalid mxf files

[Tomas Härdin]

On Wed, 2014-12-10 at 11:30 +0100, Carl Eugen Hoyos wrote:
 Hi!
 
 Attached patch fixes ticket #4173 for me.
 To be split in two parts.
 
 Please comment, Carl Eugen

Looks alright. Maybe you want to pass it a MXFMetadataSet** so you can
use av_freep() like before? But I suppose it doesn't matter since the
array gets free:d just a few lines below..

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]Fix leak reading invalid mxf files

[Tomas Härdin]

On Sat, 2014-12-13 at 13:18 +0100, Carl Eugen Hoyos wrote:
 On Friday 12 December 2014 01:43:19 pm Tomas Härdin wrote:
  On Wed, 2014-12-10 at 11:30 +0100, Carl Eugen Hoyos wrote:
   Hi!
  
   Attached patch fixes ticket #4173 for me.
   To be split in two parts.
  
   Please comment, Carl Eugen
 
  Looks alright. Maybe you want to pass it a MXFMetadataSet** 
  so you can use av_freep() like before?
 
 New patch attached that also fixes the remaining leaks from 
 ticket #4173.
 I would split in two parts, please tell me if you prefer 
 more separate commits.

Eh, it's small enough that it's clear what the patch does.

(sorry for being a bit slow with replying - I check the list about once
per week)

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/mxfdec.c: support demuxing opatom audio without index

[Tomas Härdin]

On Sun, 2014-12-21 at 17:46 -0800, Mark Reid wrote:
 hi,
 
 Media Composer generates audio OPAtom mxf files that don't have index 
 segments.
 All the uncompressed pcm audio essence data contained in a single KLV packet.
 
 This is my initial attempt to get demuxing working, I'm not too familiar with 
 demuxing
 audio or generating packets. I'm sure I'm not setting pts and dts correctly.
 Using the mxf_read_packet_old function also works too but it reads the entire
 essence KLV packet in to memory and these mxf files can be gigs.
 
 I haven't seen any raw video data encoded the same way yet, so this patch only
 enables demuxing uncompressed pcm audio mxf files without indexs.
 
 I uploaded a sample mxf encoded to ftp://upload.ffmpeg.org/incoming
 
 opatom_missing_index.mxf
 
 I can also provide more samples if need.

Have you looked at what mxf_handle_small_eubc() does? I feel this should
work similarly.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v4] libavformat/mxfdec.c: support demuxing opatom audio without index

[Tomas Härdin]

On Wed, 2015-01-14 at 19:26 -0800, Mark Reid wrote:
 changes since v3:
 * return if there isn’t exactly one partition
 * cosmetic and gcc cleanups
 * added comment about EditUnitByteCount
 
 ---
  libavformat/mxfdec.c | 59 
 
  1 file changed, 59 insertions(+)

Looks good to me :)

Maybe there could be added a FATE test for it too, I'm no longer sure
how that works.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/mxfenc: write user comment metadata

[Tomas Härdin]

On Sat, 2015-03-14 at 17:59 -0700, Mark Reid wrote:
 ---
  libavformat/mxfenc.c  | 66 
 +--
  tests/ref/lavf/mxf|  6 ++---
  tests/ref/lavf/mxf_d10|  2 +-
  tests/ref/lavf/mxf_opatom |  2 +-
  4 files changed, 69 insertions(+), 7 deletions(-)

Looks OK

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/mxfdec: export user comments metadata

[Tomas Härdin]

On Fri, 2015-03-06 at 13:24 -0800, Mark Reid wrote:
 +static int mxf_read_indirect_value(void *arg, AVIOContext *pb, int size)
 +{
 +MXFTaggedValue *tagged_value = arg;
 +uint8_t key[17];
 +
 +if (size = 17)
 +return 0;
 +
 +avio_read(pb, key, 17);

Really Avid, 17 byte keys? OK..

 +/* TODO: handle other types of of indirect values */
 +if (memcmp(key, mxf_indirect_value_utf16le, 17) == 0) {
 +return mxf_read_utf16le_string(pb, size - 17, tagged_value-value);
 +} else if (memcmp(key, mxf_indirect_value_utf16be, 17) == 0) {
 +return mxf_read_utf16be_string(pb, size - 17, tagged_value-value);
 +}
 +return 0;
 +}

 +static int mxf_parse_package_comments(MXFContext *mxf, AVDictionary **pm, 
 MXFPackage *package)
 +{
 +MXFTaggedValue *tag;
 +int size, i;
 +const char *prefix = comment_;
 +char *key = NULL;
 +
 +for (i = 0; i  package-comment_count; i++) {
 +tag = mxf_resolve_strong_ref(mxf, package-comment_refs[i], 
 TaggedValue);
 +if (!tag || !tag-name || !tag-value)
 +continue;
 +
 +size = strlen(prefix) + strlen(tag-name) + 1;
 +key = av_mallocz(size);
 +if (!key)
 +return AVERROR(ENOMEM);
 +
 +strcpy(key, prefix);
 +strlcat(key, tag-name, size);

snprintf() would make this one line only, or use av_strlcat() like
Michael suggests. Come to think of it, I'm not sure snprintf() exists on
all platforms..

 +av_dict_set(pm, key, tag-value, AV_DICT_DONT_STRDUP_KEY);
 +}
 +return 0;
 +}

Looks good overall, even if I wasn't aware of this Avid feature before.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf-body_partition_offset is not NULL before using it

[Tomas Härdin]

On Thu, 2015-03-12 at 17:48 +0100, Andreas Cadhalpun wrote:
 This fixes a crash, when trying to mux h264 into mxf_opatom.
 
 Signed-off-by: Andreas Cadhalpun andreas.cadhal...@googlemail.com
 ---
  libavformat/mxfenc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
 index 898951c..2891f5d 100644
 --- a/libavformat/mxfenc.c
 +++ b/libavformat/mxfenc.c
 @@ -2358,7 +2358,7 @@ static int mxf_write_footer(AVFormatContext *s)
  mxf_write_random_index_pack(s);
 
  if (s-pb-seekable) {
 -if (s-oformat == ff_mxf_opatom_muxer){
 +if (s-oformat == ff_mxf_opatom_muxer  
 mxf-body_partition_offset){
  /* rewrite body partition to update lengths */
  avio_seek(pb, mxf-body_partition_offset[0], SEEK_SET);
  if ((err = mxf_write_opatom_body_partition(s))  0)

Doesn't this need to happen for H.264 as well? A better solution would
be to figure out why mxf-body_partition_offset becomes NULL so that
index tables and such can be rewritten properly.

/Tomas



signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] libavformat/mxfenc: add support for muxing mxf opatom audio

[Tomas Härdin]

On Wed, 2015-03-25 at 15:43 -0700, Mark Reid wrote:
 On Mar 24, 2015 7:05 AM, Tomas Härdin tomas.har...@codemill.se wrote:
 
  On Sat, 2015-03-21 at 16:45 -0700, Mark Reid wrote:
   ---
libavformat/mxfenc.c | 100
 ++-
1 file changed, 83 insertions(+), 17 deletions(-)
 
  Looks fine as far as I can tell. My only nitpick is that it might be
  better to call timecode rate EditRate or edit rate instead, since
  that is MXF parlance. Perhaps a bit bikeshed-y though. Does anyone else
  have any preference?
 
 
 I was going to originally go with audio_edit_rate, I think its in line with
 the mxf terminology too. So I'll submit a new patch changing it.

Ah, alright

  An aside: would it ever make sense to use EditRate different from fps,
  to force a certain rate? Something tells me there's a video filter or
  something to do that already, so perhaps not an issue.
 
  /Tomas
 
 
 It might, but I'm not sure, I think avid mxf might use 30fps timecode on
 24fps footage if using pulldown or vice versa. I'll take a look. In which
 case, even this audio case, maybe the edit_rate of the timecode should be
 export as metadata so a decoding application could know what it is?

I think we can save that for when we know it's needed by some
application.

/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: Detect XYZ pixel format for digital cinema files

[Tomas Härdin]

On Thu, 2015-03-05 at 00:36 +0200, Vilius Grigaliūnas wrote:
 While the native jpeg2000 decoder can determine pixel format correctly
 from the codestream, libopenjpeg wrapper cannot. To make sure that
 the output is correct when using libopenjpeg to decode digital
 cinema files, we do detection from the metadata included in the MXF
 wrapper.
 
 If the container has JPEG 2000 Coding Parameters metadata element
 with Rsiz value set to one of digital cinema profiles, we can safely
 assume that the given input file is DCI compliant, therefore the
 pixel format should be XYZ.
 ---
  libavformat/mxfdec.c |7 +++
  1 file changed, 7 insertions(+)
 
 diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
 index f3501da..2e8dd05 100644
 --- a/libavformat/mxfdec.c
 +++ b/libavformat/mxfdec.c
 @@ -281,6 +281,7 @@ static const uint8_t mxf_encrypted_essence_container[]
  = { 0x06,0x0e,0x2b,0x
  static const uint8_t mxf_random_index_pack_key[]   = { 
 0x06,0x0e,0x2b,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x11,0x01,0x00
  };
  static const uint8_t mxf_sony_mpeg4_extradata[]= { 
 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0e,0x06,0x06,0x02,0x02,0x01,0x00,0x00
  };
  static const uint8_t mxf_avid_project_name[]   = { 
 0xa5,0xfb,0x7b,0x25,0xf6,0x15,0x94,0xb9,0x62,0xfc,0x37,0x17,0x49,0x2d,0x42,0xbf
  };
 +static const uint8_t mxf_jp2k_rsiz[]   = { 
 0x06,0x0e,0x2b,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x02,0x01,0x00
  };
  
  #define IS_KLV_KEY(x, y) (!memcmp(x, y, sizeof(y)))
  
 @@ -1000,6 +1001,12 @@ static int mxf_read_generic_descriptor(void *arg, 
 AVIOContext *pb, int tag, int
  descriptor-extradata_size = size;
  avio_read(pb, descriptor-extradata, size);
  }
 +if (IS_KLV_KEY(uid, mxf_jp2k_rsiz)) {
 +uint32_t rsiz = avio_rb16(pb);
 +if (rsiz == FF_PROFILE_JPEG2000_DCINEMA_2K ||
 +rsiz == FF_PROFILE_JPEG2000_DCINEMA_4K)
 +descriptor-pix_fmt = AV_PIX_FMT_XYZ12;
 +}
  break;
  }
  return 0;

Nice, simple and commit message explains exactly what is going on.
10/10 :)

Is there a sample file so this can be covered by FATE?

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v5] libavformat/mxfenc: write package name metadata

[Tomas Härdin]

On Wed, 2015-03-04 at 12:31 -0800, Mark Reid wrote:
 +/*
 + * Returns the calculated length a local tag containing an utf-8 string as 
 utf-16
 + */
 +static int mxf_utf16_local_tag_length(const char *utf8_str)
 +{
 +uint64_t size;
 +
 +if (!utf8_str)
 +return 0;
 +
 +size = mxf_utf16len(utf8_str);
 +if (size = UINT16_MAX || size * 2 = UINT16_MAX) {

if (size = UINT16_MAX/2) {

 +av_log(NULL, AV_LOG_ERROR, utf16 local tag size %PRIx64 invalid 
 (too large), ignoring\n, size);
 +return 0;
 +}
 +
 +return 4 + size * 2;
 +}
 +
 +/*
 + * Write a local tag containing an utf-8 string as utf-16
   */
  static void mxf_write_local_tag_utf16(AVIOContext *pb, int tag, const char 
 *value)
  {
 -int i, size = strlen(value);
 +uint64_t size = mxf_utf16len(value);
 +
 +if (size = UINT16_MAX || size * 2 = UINT16_MAX) {

if (size = UINT16_MAX/2) {

 +av_log(NULL, AV_LOG_ERROR, utf16 local tag size %PRIx64 invalid 
 (too large), ignoring\n, size);
 +return;
 +}
 +
  mxf_write_local_tag(pb, size*2, tag);
 -for (i = 0; i  size; i++)
 -avio_wb16(pb, value[i]);
 +avio_put_str16be(pb, value);
  }

The rest looks OK to me. Nice work :)

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2] libavformat/mxfenc: added support mxf opatom audio muxing

[Tomas Härdin]

On Fri, 2015-04-10 at 20:02 -0700, Mark Reid wrote:
 @@ -2055,8 +2083,35 @@ static int mxf_write_header(AVFormatContext *s)
  av_log(s, AV_LOG_ERROR, MXF D-10 only support 16 or 24 
 bits le audio\n);
  }
  sc-index = 
 ((MXFStreamContext*)s-streams[0]-priv_data)-index + 1;
 -} else
 -mxf-slice_count = 1;
 +} else if (s-oformat == ff_mxf_opatom_muxer) {
 +AVRational tbc = av_inv_q(mxf-audio_edit_rate);
 +
 +if (st-codec-codec_id != AV_CODEC_ID_PCM_S16LE 
 +st-codec-codec_id != AV_CODEC_ID_PCM_S24LE) {
 +av_log(s, AV_LOG_ERROR, MXF OPAtom only supports 16 or 
 24 bits le audio\n);

This should perhaps be reworded as Only 16- and 24-bit LE is
implemented since the spec itself allows loads more.

 +return AVERROR(EINVAL);
 +}
 +if (st-codec-channels != 1) {
 +av_log(s, AV_LOG_ERROR, MXF OPAtom only supports single 
 channel audio\n);

Similar here.

Looks OK otherwise.

/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mxfenc: Add color siting element

[Tomas Härdin]

On Tue, 2015-05-19 at 15:35 +0100, tim nicholson wrote:
 On 19/05/15 14:11, Michael Niedermayer wrote:
  On Tue, May 19, 2015 at 12:07:24PM +0100, tim nicholson wrote:
  On 19/05/15 01:33, Michael Niedermayer wrote:
  The default is assumed to be 0xFF, which is what the 2009 spec lists
 ,
  the older version i have lists 0 as the default.
 
  Signed-off-by: Michael Niedermayer michae...@gmx.at
  ---
   libavformat/mxfenc.c|   28 +
   tests/ref/lavf/mxf  |   12 +--
   tests/ref/lavf/mxf_d10  |2 +-
   tests/ref/lavf/mxf_opatom   |2 +-
   tests/ref/lavf/mxf_opatom_audio |2 +-
   tests/ref/seek/lavf-mxf |   44 +++-
 ---
   6 files changed, 59 insertions(+), 31 deletions(-)
 
  diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
  index 659c34f..0af3db1 100644
  --- a/libavformat/mxfenc.c
  +++ b/libavformat/mxfenc.c
  @@ -79,6 +79,7 @@ typedef struct MXFStreamContext {
   int interlaced;  /// whether picture is interlaced
   int field_dominance; /// tff=1, bff=2
   int component_depth;
  +int color_siting;
   int h_chroma_sub_sample;
   int temporal_reordering;
   AVRational aspect_ratio; /// display aspect ratio
  @@ -416,6 +417,7 @@ static const MXFLocalTagPair mxf_local_tag_batch
 [] = {
   // CDCI Picture Essence Descriptor
   { 0x3301, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x04,0x01,0x
 05,0x03,0x0A,0x00,0x00,0x00}}, /* Component Depth */
   { 0x3302, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x01,0x04,0x01,0x
 05,0x01,0x05,0x00,0x00,0x00}}, /* Horizontal Subsampling */
  +{ 0x3303, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x01,0x04,0x01,0x
 05,0x01,0x06,0x00,0x00,0x00}}, /* Color Siting */
   // Generic Sound Essence Descriptor
   { 0x3D02, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x04,0x04,0x02,0x
 03,0x01,0x04,0x00,0x00,0x00}}, /* Locked/Unlocked */
   { 0x3D03, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x05,0x04,0x02,0x
 03,0x01,0x01,0x01,0x00,0x00}}, /* Audio sampling rate */
  @@ -996,6 +998,8 @@ static void mxf_write_cdci_common(AVFormatContex
 t *s, AVStream *st, const UID ke
   unsigned desc_size = size+8+8+8+8+8+8+8+5+16+sc-interlaced*4+1
 2+20;
   if (sc-interlaced  sc-field_dominance)
   desc_size += 5;
  +if (sc-color_siting != 0xFF)
  +desc_size += 5;
   
   mxf_write_generic_desc(s, st, key, desc_size);
   
  @@ -1030,6 +1034,12 @@ static void mxf_write_cdci_common(AVFormatCon
 text *s, AVStream *st, const UID ke
   mxf_write_local_tag(pb, 4, 0x3302);
   avio_wb32(pb, sc-h_chroma_sub_sample);
   
  +if (sc-color_siting != 0xFF) {
  +// color siting
  +mxf_write_local_tag(pb, 1, 0x3303);
  +avio_w8(pb, sc-color_siting);
  +}
  +
 
  If the value as far as we can determine is unknown, should we not
  write that value (0xFF), rather than leave the metadata out, and hope
  that any reader will assume the 2009 default rather than the previous
 ,
  different default? It would seem to me to be more universal.
 
  I'm generally wary of leaving out values just because they are some
  default especially if that default is subject to change.
  
  i was trying to avoid breaking things by favoring not writing it
  as is done in git currently.
  But maybe iam too carefull here, do you think we can safely write
  0xFF always ? (which could happen if the user does not provide any
  information about the chroma location or pixel format
 
 I should say so, but I wonder if Tomas has a view.
 
  
  or should this be tested with some applications? if so with what
  applications ?

As patch submitter I assume you have an application in mind. MXF isn't
really something you just implement stuff for on a lark. What's your use
case? What's your test for that use case?

 Currently bmxlib reports an ffmpeg.mxf as:-
 
 color_siting: Unknown (value='255')
 
 i.e, in the absence of the metadata entry it assumes the correct
 default, so forcibly setting it will make no difference here (or with
 any other up to date reader). Its only for anything conforming to the
 old standard of which I do not know a sample.

Sounds reasonable to me.

  [...]
 
  +case AVCHROMA_LOC_UNSPECIFIED:
  +if (pix_desc) {
  +if (pix_desc-log2_chroma_h == 0) {
  +sc-color_siting = 0;
  +} else if (pix_desc-log2_chroma_w == 1  pix_
 desc-log2_chroma_h == 1) {
  +switch (st-codec-codec_id) {
  +case AV_CODEC_ID_MPEG2VIDEO: sc-color_siti
 ng = 6; break;
 
  Only true for 4:2:0 mpeg2, what about 4:2:2 (XDCAM-HD) or does the 'i
 f'
  filter that out? (I'm not that familiar with the pix_desc struct).
  
  the if implies 4:2:0
  
 
 Ah fine then.

Why is this guessing code in mxfenc? This should be something that's
taken care of before calling any muxer (like in

Re: [FFmpeg-devel] [PATCH] mxfdec: set AVFMT_SEEK_TO_PTS demuxer flag

[Tomas Härdin]

On Mon, 2015-08-10 at 10:14 +0200, Tomas Härdin wrote:
 On Sun, 2015-08-09 at 20:32 +0200, Marton Balint wrote:
  Since 53f2ef2c4afb1d49a679dea9163cb0e4671f3117 seeking is done using PTS.
  
  Signed-off-by: Marton Balint c...@passwd.hu
  ---
   libavformat/mxfdec.c | 1 +
   1 file changed, 1 insertion(+)
  
  diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
  index 2d921db..5734976 100644
  --- a/libavformat/mxfdec.c
  +++ b/libavformat/mxfdec.c
  @@ -3210,6 +3210,7 @@ static int mxf_read_seek(AVFormatContext *s, int 
  stream_index, int64_t sample_ti
   AVInputFormat ff_mxf_demuxer = {
   .name   = mxf,
   .long_name  = NULL_IF_CONFIG_SMALL(MXF (Material eXchange 
  Format)),
  +.flags  = AVFMT_SEEK_TO_PTS,
   .priv_data_size = sizeof(MXFContext),
   .read_probe = mxf_probe,
   .read_header= mxf_read_header,
 
 Yeah, I seem to recall this when swearing at the seek code in mxfdec
 some years ago. I'll wait a few days to see if any other MXF guys have
 something to say here or on IRC, then I suppose I'll push
 
 /Tomas

Pushed. Hopefully everything worked alright

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] mxfdec: calculate the index in display order

[Tomas Härdin]

On Sat, 2015-07-11 at 18:54 +0200, Marton Balint wrote:
 This should fix seeking for open GOP files as well.
 
 Signed-off-by: Marton Balint c...@passwd.hu
 ---
 @@ -1411,8 +1417,7 @@ static int mxf_compute_ptses_fake_index(MXFContext 
 *mxf, MXFIndexTable *index_ta
  break;
  }
  
 -index_table-fake_index[x].timestamp = x;

Why is this removed?

 -index_table-fake_index[x].flags = !(s-flag_entries[j]  0x30) 
 ? AVINDEX_KEYFRAME : 0;
 +flags[x] = !(s-flag_entries[j]  0x30) ? AVINDEX_KEYFRAME : 0;
  
  if (index  0 || index = index_table-nb_ptses) {
  av_log(mxf-fc, AV_LOG_ERROR,
 @@ -1421,11 +1426,20 @@ static int mxf_compute_ptses_fake_index(MXFContext 
 *mxf, MXFIndexTable *index_ta
  continue;
  }
  
 +index_table-offsets[x] = offset;
  index_table-ptses[index] = x;
  max_temporal_offset = FFMAX(max_temporal_offset, offset);
  }
  }
  
 +/* calculate the fake index table in display order */
 +for (x = 0; x  index_table-nb_ptses; x++) {
 +index_table-fake_index[x].timestamp = x;
 +if (index_table-ptses[x] != AV_NOPTS_VALUE)
 +index_table-fake_index[index_table-ptses[x]].flags = flags[x];

ptses are checked to be in range, right?

Anyway, the patch is probably OK. I recall doing something similar at
work in another mxf library. Do we have test files with B-frames? If not
then this may be a good time to add them.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/mxf: Map codec_tag for Avid files if everything else fails

[Tomas Härdin]

On Fri, 2015-07-17 at 12:36 +0200, Carl Eugen Hoyos wrote:
 On Saturday 11 July 2015 04:13:52 pm Tomas Härdin wrote:
  Just a quick review since I have to bounce:
   +const MXFCodecUL ff_mxf_codec_tag_uls[] = {
 
  Haven't we moved this to mxf.c already? Or rather, don't we 
  have a whole bunch of very similar tables already?
 
 The new table is (together with others) in mxf.c, none of the 
 existing tables maps to codec_tag. AVup cannot be decoded 
 without codec_tag because we currently treat it as rawvideo.

Alright

 [...]
 
  Messy bracing. Something like putting a check on codec_tag after setting
  it, inside braces like before. Hard to explain and I don't have time to
  type it out but:
 
  if (st-codec-pix_fmt == AV_PIX_FMT_NONE) {
   codec_tag = ...
   if (!codec_tag) {
do the old thing
   }
  }
 
 New patch attached.

Looks alright. You can reindent in a separate patch if you like.

Sorry for taking a few days with replying, thought I'd already sorted it

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 01/11] libavformat/mxfdec.c: klv_read_packet: Properly check klv_decode_ber_length return value.

[Tomas Härdin]

On Thu, 2015-10-22 at 19:25 +0200, Alexis Ballier wrote:
> On Wed, 21 Oct 2015 23:31:29 +0200
> Tomas Härdin <tomas.har...@codemill.se> wrote:
> 
> > On Wed, 2015-10-21 at 18:00 +0200, Alexis Ballier wrote:
> > > klv_decode_ber_length cannot return -1, but can return
> > > AVERROR_INVALIDDATA. Store its return value in a signed integer
> > > (instead of unsigned KLVPacket.length) and forward the error
> > > appropriately. --- libavformat/mxfdec.c | 6 --
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index 00d420b..94a953b 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -366,13 +366,15 @@ static int mxf_read_sync(AVIOContext *pb,
> > > const uint8_t *key, unsigned size) 
> > >  static int klv_read_packet(KLVPacket *klv, AVIOContext *pb)
> > >  {
> > > +int64_t len;
> > >  if (!mxf_read_sync(pb, mxf_klv_key, 4))
> > >  return AVERROR_INVALIDDATA;
> > >  klv->offset = avio_tell(pb) - 4;
> > >  memcpy(klv->key, mxf_klv_key, 4);
> > >  avio_read(pb, klv->key + 4, 12);
> > > -klv->length = klv_decode_ber_length(pb);
> > > -return klv->length == -1 ? -1 : 0;
> > > +len = klv_decode_ber_length(pb);
> > > +klv->length = FFMAX(len, 0);
> > > +return FFMIN(len, 0);
> > >  }  
> > 
> > Can't klv_read_packet() return int64_t instead?
> 
> you mean change the return type and return klv->length ?
> that might work, but note that what I'm trying to fix is
> klv_read_packet setting length to (unsigned)AVERROR_INVALIDDATA, i.e.,
> something close to 2^64, and returning 0.
> I don't see any problem by doing that, so it is as you prefer.

Oh, of course - don't let length be some bogus value :)
Actually, maybe it doesn't matter so much. Hm

/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 04/11] libavformat/mxfdec.c: Try to increment current edit before rejecting a klv that spans onto next edit unit.

[Tomas Härdin]

On Thu, 2015-10-22 at 19:47 +0200, Alexis Ballier wrote:
> On Wed, 21 Oct 2015 23:45:07 +0200
> Tomas Härdin <tomas.har...@codemill.se> wrote:
> 
> > On Wed, 2015-10-21 at 18:00 +0200, Alexis Ballier wrote:
> > > Some files such as those from tickets #2817 & #2776 claim to have
> > > constant edit unit size but, in fact, have some of them that are
> > > smaller. This confuses the demuxer that tries to infer the current
> > > edit unit from the position in the file. By trying to increment the
> > > current edit unit before rejecting the packet for this reason, we
> > > try to make it fit into its proper edit unit, which fixes demuxing
> > > of those files while preserving the check for misprobed OpAtom
> > > files. Seeking is not accurate but the files provide no way to
> > > properly find the relevant edit unit.
> > > 
> > > Fixes tickets #2817 & #2776.
> > > ---
> > >  libavformat/mxfdec.c | 12 
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index 593604e..526eca6 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -2956,6 +2956,18 @@ static int
> > > mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt) next_ofs =
> > > mxf_set_current_edit_unit(mxf, klv.offset); 
> > >  if (next_ofs >= 0 && next_klv > next_ofs) {
> > > +/* Samples from tickets #2817 and #2776 claim to
> > > have
> > > + * constant edit unit size. However, some of them
> > > are smaller.  
> > 
> > What does "them" refer to here? The edit units or the KLVs?
> > 
> > > + * Just after those smaller edit units,  
> > 
> > Right, the edit units. Maybe rework the grammar slightly.
> > 
> > > + * Just after those smaller edit units, klv.offset
> > > is still in
> > > + * the same edit unit according to the
> > > computations from the
> > > + * constant edit unit size. If the klv finishes
> > > after, the next
> > > + * check would truncate the packet and prevent
> > > proper demuxing.
> > > + * Try to increment the current edit unit before
> > > doing that. */  
> > 
> > Let's see if I understand this correctly. For say EUBC = 10, there can
> > still be KLVs that are some size larger than 10, but smaller than
> > 2*EUBC = 20? So that the next edit unit would extend past the end of
> > the KLV, and thus be bogus?
> > 
> >   KLV: |header|---|header|--|
> > Edit unit:|0123456789|bogus<10|  |0123456789|bgs|
> > 
> > IIRC with MXF the bogus parts should still count as part of the
> > essence stream. Maybe I'm missing something.
> 
> It's simpler than that, and if you don't understand then the comment
> likely needs improving :) let's see:
> 
> H = header, V = video, A,B,C = audio tracks, F = fill item
> 
> mxf file defines a proper edit unit, with EUBC = 10 to be something
> like:
> 
> 1234567890
> HVVVAFBFCF
> 
> now, in the samples, in some edit units, video is shorter; mxf spec
> says it should be padded by fill items, but they're not and look like:
> 
> 1234567890
> HVAFBFCF
> 
> when continuing to read, we have:
> 
> 12345678901234567890
> HVAFBFCFHVVVAFBFCF
> | eu 1   || eu 2   |
> 
> as you can see, 2nd video packet is still in the first edit unit
> according to EUBC, and extends to next one.

Ah, that makes it a lot clearer :)

> that's what the patch is about: try to increment edit unit before
> rejecting the packet.
> 
> in 'MXF_DVCAM_not_demuxable.mxf', those smaller video packets seem to
> correspond to a black frame inserted between two scenes.
> 
> I've tried hard to get something better, but nothing seemed to work
> properly; best other option I had was to increment edit unit when
> seeing a system item, which worked but broke tests and in which I'm not
> so confident it won't break with other broken files...

Yeah, breaking existing tests is obviously not OK. But increasing
current_edit_unit like that seems a bit too suspect.

What your patch seems to end up doing with that
max_set_current_edit_unit() call is call mxf_edit_unit_absolute_offset()
like:

mxf_edit_unit_absolute_offset(mxf, t, mxf->current_edit_unit + 1, NULL, 
_ofs, 0)

Maybe you could make use of just that function call (with
mxf->current_edit_unit + *2*), instead of potentially messing
current_edit_unit up for some corner cases..

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 02/11] libavformat/mxfdec.c: cosmetics: Add missing space after '?' in log message.

[Tomas Härdin]

On Wed, 2015-10-21 at 18:00 +0200, Alexis Ballier wrote:
> ---
>  libavformat/mxfdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 94a953b..0ae7ce6 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -2959,7 +2959,7 @@ static int mxf_read_packet_old(AVFormatContext *s, 
> AVPacket *pkt)
>  /* if this check is hit then it's possible OPAtom was 
> treated as OP1a
>   * truncate the packet since it's probably very large (>2 
> GiB is common) */
>  avpriv_request_sample(s,
> -  "OPAtom misinterpreted as OP1a?"
> +  "OPAtom misinterpreted as OP1a? "
>"KLV for edit unit %i extending into "
>"next edit unit",
>mxf->current_edit_unit);

Obviously fine

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 01/11] libavformat/mxfdec.c: klv_read_packet: Properly check klv_decode_ber_length return value.

[Tomas Härdin]

On Wed, 2015-10-21 at 18:00 +0200, Alexis Ballier wrote:
> klv_decode_ber_length cannot return -1, but can return AVERROR_INVALIDDATA.
> Store its return value in a signed integer (instead of unsigned 
> KLVPacket.length) and forward the error appropriately.
> ---
>  libavformat/mxfdec.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 00d420b..94a953b 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -366,13 +366,15 @@ static int mxf_read_sync(AVIOContext *pb, const uint8_t 
> *key, unsigned size)
>  
>  static int klv_read_packet(KLVPacket *klv, AVIOContext *pb)
>  {
> +int64_t len;
>  if (!mxf_read_sync(pb, mxf_klv_key, 4))
>  return AVERROR_INVALIDDATA;
>  klv->offset = avio_tell(pb) - 4;
>  memcpy(klv->key, mxf_klv_key, 4);
>  avio_read(pb, klv->key + 4, 12);
> -klv->length = klv_decode_ber_length(pb);
> -return klv->length == -1 ? -1 : 0;
> +len = klv_decode_ber_length(pb);
> +klv->length = FFMAX(len, 0);
> +return FFMIN(len, 0);
>  }

Can't klv_read_packet() return int64_t instead?

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 04/11] libavformat/mxfdec.c: Try to increment current edit before rejecting a klv that spans onto next edit unit.

[Tomas Härdin]

On Wed, 2015-10-21 at 18:00 +0200, Alexis Ballier wrote:
> Some files such as those from tickets #2817 & #2776 claim to have constant 
> edit unit size but,
> in fact, have some of them that are smaller. This confuses the demuxer that 
> tries to infer the
> current edit unit from the position in the file. By trying to increment the 
> current edit unit
> before rejecting the packet for this reason, we try to make it fit into its 
> proper edit unit,
> which fixes demuxing of those files while preserving the check for misprobed
> OpAtom files.
> Seeking is not accurate but the files provide no way to properly find the 
> relevant edit unit.
> 
> Fixes tickets #2817 & #2776.
> ---
>  libavformat/mxfdec.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 593604e..526eca6 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -2956,6 +2956,18 @@ static int mxf_read_packet_old(AVFormatContext *s, 
> AVPacket *pkt)
>  next_ofs = mxf_set_current_edit_unit(mxf, klv.offset);
>  
>  if (next_ofs >= 0 && next_klv > next_ofs) {
> +/* Samples from tickets #2817 and #2776 claim to have
> + * constant edit unit size. However, some of them are 
> smaller.

What does "them" refer to here? The edit units or the KLVs?

> + * Just after those smaller edit units,

Right, the edit units. Maybe rework the grammar slightly.

> + * Just after those smaller edit units, klv.offset is still 
> in
> + * the same edit unit according to the computations from the
> + * constant edit unit size. If the klv finishes after, the 
> next
> + * check would truncate the packet and prevent proper 
> demuxing.
> + * Try to increment the current edit unit before doing that. 
> */

Let's see if I understand this correctly. For say EUBC = 10, there can
still be KLVs that are some size larger than 10, but smaller than 2*EUBC
= 20? So that the next edit unit would extend past the end of the KLV,
and thus be bogus?

  KLV: |header|---|header|--|
Edit unit:|0123456789|bogus<10|  |0123456789|bgs|

IIRC with MXF the bogus parts should still count as part of the essence
stream. Maybe I'm missing something.

> +mxf->current_edit_unit++;
> +next_ofs = mxf_set_current_edit_unit(mxf, klv.offset);

I feel like this should be more of an explicit check, else maybe it can
miss edit units for some corner cases?

Hm-hm.. I'm noticing I'm very confused. This is very typical of MXF.
I'll take another look at this in the morning.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] libavformat/mxfenc.c: Fix segfault when writing an audio packet when there has not been a video one.

[Tomas Härdin]

On Tue, 2015-10-20 at 16:43 +0200, Marton Balint wrote:
> On Mon, 19 Oct 2015, Tomas Härdin wrote:
> 
> > On Mon, 2015-10-19 at 11:40 +0200, Alexis Ballier wrote:
> >> On Mon, 19 Oct 2015 10:30:00 +0200
> >> Michael Niedermayer <mich...@niedermayer.cc> wrote:
> >>
> >>> On Fri, Oct 16, 2015 at 10:42:32AM +0200, Alexis Ballier wrote:
> >>>> This happens when writing the trailer of a file containing audio
> >>>> but that has not muxed any video packet. Fixes ticket #4817.
> >>>>
> >>>> 
> >>>
> >>> from IRC:
> >>>  maybe it should print a warning that there has been no
> >>> video?
> >>
> >> maybe, but what would be the right condition ? just this case ? or when
> >> writing any edit unit without video? or... ?
> >> I haven't been able to find clear references on this (well, those smpte
> >> specs I googled aren't esp. easy to read either), but e.g. the muxer
> >> refuses to mux without exactly one video stream, so maybe, an empty
> >> video stream is also an error; I don't know
> >
> > I'm not particularly fond of second-guessing these kinds of things. D-10
> > is really anal about what constitutes a legal file, so it's probably
> > best not to write anything if something seems amiss. In the end, the
> > real test for correctness is whether the output works with all gear for
> > your particular use case. But segfaults are important to fix, so I'm not
> > too concerned here. Plus my main concern is mxfdec, not mxfenc.
> >
> 
> Why not simply return error and print an error explaining "An audio packet 
> was muxed before a video, this is not supported."
> 
> Segfault -> error is much better than segfault -> possibly broken file.

Yes, simply bailing out is infinitely better than segfaulting. It's also
the more conservative approach, which I am for

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 04/11] libavformat/mxfdec.c: Try to increment current edit before rejecting a klv that spans onto next edit unit.

[Tomas Härdin]

On Sun, 2015-10-25 at 21:43 +0100, Tomas Härdin wrote:
> On Thu, 2015-10-22 at 19:47 +0200, Alexis Ballier wrote:
> > On Wed, 21 Oct 2015 23:45:07 +0200
> > Tomas Härdin <tomas.har...@codemill.se> wrote:
> > 
> > > On Wed, 2015-10-21 at 18:00 +0200, Alexis Ballier wrote:
> > > > Some files such as those from tickets #2817 & #2776 claim to have
> > > > constant edit unit size but, in fact, have some of them that are
> > > > smaller. This confuses the demuxer that tries to infer the current
> > > > edit unit from the position in the file. By trying to increment the
> > > > current edit unit before rejecting the packet for this reason, we
> > > > try to make it fit into its proper edit unit, which fixes demuxing
> > > > of those files while preserving the check for misprobed OpAtom
> > > > files. Seeking is not accurate but the files provide no way to
> > > > properly find the relevant edit unit.
> > > > 
> > > > Fixes tickets #2817 & #2776.
> > > > ---
> > > >  libavformat/mxfdec.c | 12 
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > > index 593604e..526eca6 100644
> > > > --- a/libavformat/mxfdec.c
> > > > +++ b/libavformat/mxfdec.c
> > > > @@ -2956,6 +2956,18 @@ static int
> > > > mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt) next_ofs =
> > > > mxf_set_current_edit_unit(mxf, klv.offset); 
> > > >  if (next_ofs >= 0 && next_klv > next_ofs) {
> > > > +/* Samples from tickets #2817 and #2776 claim to
> > > > have
> > > > + * constant edit unit size. However, some of them
> > > > are smaller.  
> > > 
> > > What does "them" refer to here? The edit units or the KLVs?
> > > 
> > > > + * Just after those smaller edit units,  
> > > 
> > > Right, the edit units. Maybe rework the grammar slightly.
> > > 
> > > > + * Just after those smaller edit units, klv.offset
> > > > is still in
> > > > + * the same edit unit according to the
> > > > computations from the
> > > > + * constant edit unit size. If the klv finishes
> > > > after, the next
> > > > + * check would truncate the packet and prevent
> > > > proper demuxing.
> > > > + * Try to increment the current edit unit before
> > > > doing that. */  
> > > 
> > > Let's see if I understand this correctly. For say EUBC = 10, there can
> > > still be KLVs that are some size larger than 10, but smaller than
> > > 2*EUBC = 20? So that the next edit unit would extend past the end of
> > > the KLV, and thus be bogus?
> > > 
> > >   KLV: |header|---|header|--|
> > > Edit unit:|0123456789|bogus<10|  |0123456789|bgs|
> > > 
> > > IIRC with MXF the bogus parts should still count as part of the
> > > essence stream. Maybe I'm missing something.
> > 
> > It's simpler than that, and if you don't understand then the comment
> > likely needs improving :) let's see:
> > 
> > H = header, V = video, A,B,C = audio tracks, F = fill item
> > 
> > mxf file defines a proper edit unit, with EUBC = 10 to be something
> > like:
> > 
> > 1234567890
> > HVVVAFBFCF
> > 
> > now, in the samples, in some edit units, video is shorter; mxf spec
> > says it should be padded by fill items, but they're not and look like:
> > 
> > 1234567890
> > HVAFBFCF
> > 
> > when continuing to read, we have:
> > 
> > 12345678901234567890
> > HVAFBFCFHVVVAFBFCF
> > | eu 1   || eu 2   |
> > 
> > as you can see, 2nd video packet is still in the first edit unit
> > according to EUBC, and extends to next one.
> 
> Ah, that makes it a lot clearer :)
> 
> > that's what the patch is about: try to increment edit unit before
> > rejecting the packet.
> > 
> > in 'MXF_DVCAM_not_demuxable.mxf', those smaller video packets seem to
> > correspond to a black frame inserted between two scenes.
> > 
> > I've tried hard to get something better, but nothing seemed to work
> > properly; best other option I had was to increment edit unit when
> > seeing a system item, which 

Re: [FFmpeg-devel] [PATCH 1/2] libavformat/mxfenc.c: Fix segfault when writing an audio packet when there has not been a video one.

[Tomas Härdin]

On Mon, 2015-10-19 at 11:40 +0200, Alexis Ballier wrote:
> On Mon, 19 Oct 2015 10:30:00 +0200
> Michael Niedermayer  wrote:
> 
> > On Fri, Oct 16, 2015 at 10:42:32AM +0200, Alexis Ballier wrote:
> > > This happens when writing the trailer of a file containing audio
> > > but that has not muxed any video packet. Fixes ticket #4817.
> > > 
> > >   
> > 
> > from IRC:
> >  maybe it should print a warning that there has been no
> > video?
> 
> maybe, but what would be the right condition ? just this case ? or when
> writing any edit unit without video? or... ?
> I haven't been able to find clear references on this (well, those smpte
> specs I googled aren't esp. easy to read either), but e.g. the muxer
> refuses to mux without exactly one video stream, so maybe, an empty
> video stream is also an error; I don't know

I'm not particularly fond of second-guessing these kinds of things. D-10
is really anal about what constitutes a legal file, so it's probably
best not to write anything if something seems amiss. In the end, the
real test for correctness is whether the output works with all gear for
your particular use case. But segfaults are important to fix, so I'm not
too concerned here. Plus my main concern is mxfdec, not mxfenc.

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [RFC] avformat/mxfenc: stop encoding if unfilled video packet

[Tomas Härdin]

On Mon, 2015-10-05 at 14:25 +0200, Tobias Rapp wrote:
> On 05.10.2015 09:10, tim nicholson wrote:
> > On 04/10/15 13:07, Tomas Härdin wrote:
> >> On Mon, 2015-09-28 at 15:11 +0200, Tobias Rapp wrote:
> >>> [...]
> >>
> >> For me the most important thing is that anything dealing with MXF should
> >> never write invalid files.
> >
> > +1 for sure.
> 
> To overstate your point: So it would be OK to skip most input frames and 
> replace them with black frames as long as the output file is valid MXF?
> 
> I think there are different requirements for determining what makes an 
> "error" depending on the use-case. If I try to recover video frames from 
> an broken hard disk, for example, I probably won't mind some lost 
> frames. If I try to encode a video file from a presumably healthy input 
> file I likely care about lost frames.
> 
> >> I'm not sure whether the current code is
> >> broken per se, but it does look suspicious. Perhaps someone else has a
> >> better idea?
> >>
> >
> > Truncate/pad mis sized frames to allow muxing to continue, and issue a
> > warning (as at present)?
> 
> It is currently quite difficult to ensure that all frames have been 
> transcoded if there is only a warning in the log message because AFAIK 
> the only generic way to separate a info log message from a warning is to 
> parse terminal color code sequences. The other solution would be to 
> maintain a RegEx black-list of log output messages in the parent process.
> 
> Wouldn't it be helpful to introduce some flag option like "-flags 
> +xerror" on avformat level to toggle between "recover as much as 
> possible" mode and "encode all or fail" mode?

Possibly. Feels like it'd be tricky to write tests for. But I do see a
point in having a mode where lavf is not allowed to do anything strange,
as it often tends to want to do. But this really touches on some rather
nasty bits of how lavf is (not) structured, and I think it might be
easier if you just keep some local hack for your workflow rather than
try to bring some sanity to the situation..

/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/mxf: Map codec_tag for Avid files if everything else fails

[Tomas Härdin]

On Fri, 2015-07-10 at 20:11 +0200, Carl Eugen Hoyos wrote:
 Hi!
 
 Attached patch allows decoding of the sample provided on ffmpeg-user:
 http://ffmpeg.org/pipermail/ffmpeg-user/2015-July/027472.html
 
 Better suggestions welcome, Carl Eugen

Just a quick review since I have to bounce:

 +const MXFCodecUL ff_mxf_codec_tag_uls[] = {

Haven't we moved this to mxf.c already? Or rather, don't we have a whole
bunch of very similar tables already?

  st-codec-pix_fmt = (enum AVPixelFormat)pix_fmt_ul-id;
 -if (st-codec-pix_fmt == AV_PIX_FMT_NONE) {
 +if (st-codec-pix_fmt == AV_PIX_FMT_NONE)
 +st-codec-codec_tag = 
 mxf_get_codec_ul(ff_mxf_codec_tag_uls,
 +
 descriptor-essence_codec_ul)-id;
 +if (st-codec-pix_fmt == AV_PIX_FMT_NONE  
 !st-codec-codec_tag) {
  /* support files created before RP224v10 by 
 defaulting to UYVY422
 if subsampling is 4:2:2 and component depth is 
 8-bit */
  if (descriptor-horiz_subsampling == 2 

Messy bracing. Something like putting a check on codec_tag after setting
it, inside braces like before. Hard to explain and I don't have time to
type it out but:

if (st-codec-pix_fmt == AV_PIX_FMT_NONE) {
 codec_tag = ...
 if (!codec_tag) {
  do the old thing
 }
}

Gotta go!

/Tomas



___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] mxfdec: set AVFMT_SEEK_TO_PTS demuxer flag

[Tomas Härdin]

On Tue, 2015-08-25 at 07:58 +0100, tim nicholson wrote:
 On 14/08/15 13:27, Tomas Härdin wrote:
  On Mon, 2015-08-10 at 10:14 +0200, Tomas Härdin wrote:
  On Sun, 2015-08-09 at 20:32 +0200, Marton Balint wrote:
  Since 53f2ef2c4afb1d49a679dea9163cb0e4671f3117 seeking is done using
  PTS.
 
  Signed-off-by: Marton Balint c...@passwd.hu
  ---
   libavformat/mxfdec.c | 1 +
   1 file changed, 1 insertion(+)
 
  diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
  index 2d921db..5734976 100644
  --- a/libavformat/mxfdec.c
  +++ b/libavformat/mxfdec.c
  @@ -3210,6 +3210,7 @@ static int mxf_read_seek(AVFormatContext *s, i
 nt stream_index, int64_t sample_ti
   AVInputFormat ff_mxf_demuxer = {
   .name   = mxf,
   .long_name  = NULL_IF_CONFIG_SMALL(MXF (Material eXchange 
 Format)),
  +.flags  = AVFMT_SEEK_TO_PTS,
   .priv_data_size = sizeof(MXFContext),
   .read_probe = mxf_probe,
   .read_header= mxf_read_header,
 
  Yeah, I seem to recall this when swearing at the seek code in mxfdec
  some years ago. I'll wait a few days to see if any other MXF guys hav
 e
  something to say here or on IRC, then I suppose I'll push
 
 
 Only the seek code you swear at?

Good point; MXF provides plenty of opportunity for getting riled up

  /Tomas
  
  Pushed. Hopefully everything worked alright
  
 
 Just back from leave, so time will tell

Indeed.

/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [RFC] avformat/mxfenc: stop encoding if unfilled video packet

[Tomas Härdin]

On Mon, 2015-09-28 at 15:11 +0200, Tobias Rapp wrote:
> On 19.09.2015 22:49, Tomas Härdin wrote:
> > On Wed, 2015-09-16 at 14:33 +0200, Tobias Rapp wrote:
> >> Hi,
> >>
> >> attached patch fixes ticket #4759 but I guess it is a bit too hasty to
> >> always abort transcoding if a single frame cannot be written. I guess it
> >> would be better to check for some "exit_on_error" like flag set but
> >> couldn't find out how to achieve that.
> >>
> >> Any comments would be appreciated.
> >>
> >> Regards,
> >> Tobias
> >
> >
> >>  From 7d6f8de2a411817c970a19d8766e69b6eb604132 Mon Sep 17 00:00:00 2001
> >> From: Tobias Rapp <t.r...@noa-audio.com>
> >> Date: Mon, 14 Sep 2015 12:06:22 +0200
> >> Subject: [PATCH] avformat/mxfenc: stop encoding if unfilled video packet
> >>   occurs
> >>
> >> Fixes ticket #4759.
> >> ---
> >>   libavformat/mxfenc.c | 14 +-
> >>   1 file changed, 9 insertions(+), 5 deletions(-)
> >>
> [...]
> >
> > Is this really better than not writing anything?
> >
> > /Tomas
> 
> (Sorry for the long delay in answering, I was caught by a flu last week.)
> 
> I want to transcode thousands of files and want to get some indication 
> from ffmpeg if the transcoding has been successful (all frames have been 
> transcoded) or not. Currently I am using the process exit code to verify 
> that.
> 
> There are two different user stories when using ffmpeg:
> a) "process the input data and exit with error as early as possible in 
> case of errors/problems"
> b) "process the input data and avoid exit with error as long as possible 
> in case of error/problems"
> 
> If I understand correctly I can basically switch between (a) and (b) 
> mode by passing the "-xerror" option to ffmpeg or not. So my plan is to 
> transcode all my files with "-xerror" and assume that >99% of the files 
> will pass. The small amount of failing ones can then be analyzed for 
> problems manually and possibly have a re-run without "-xerror".
> 
> Unfortunately the "-xerror" option affects only ffmpeg but not the 
> libraries, so I cannot use it do decide if "mxf_write_packet" should 
> return with an error when the video packet size doesn't match the CBR 
> constraints.

For me the most important thing is that anything dealing with MXF should
never write invalid files. I'm not sure whether the current code is
broken per se, but it does look suspicious. Perhaps someone else has a
better idea?

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [RFC] avformat/mxfenc: stop encoding if unfilled video packet

[Tomas Härdin]

On Wed, 2015-09-16 at 14:33 +0200, Tobias Rapp wrote:
> Hi,
> 
> attached patch fixes ticket #4759 but I guess it is a bit too hasty to 
> always abort transcoding if a single frame cannot be written. I guess it 
> would be better to check for some "exit_on_error" like flag set but 
> couldn't find out how to achieve that.
> 
> Any comments would be appreciated.
> 
> Regards,
> Tobias


> From 7d6f8de2a411817c970a19d8766e69b6eb604132 Mon Sep 17 00:00:00 2001
> From: Tobias Rapp 
> Date: Mon, 14 Sep 2015 12:06:22 +0200
> Subject: [PATCH] avformat/mxfenc: stop encoding if unfilled video packet
>  occurs
> 
> Fixes ticket #4759.
> ---
>  libavformat/mxfenc.c | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 84ce979..4eac812 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2262,7 +2262,7 @@ static void mxf_write_system_item(AVFormatContext *s)
>  mxf_write_umid(s, 1);
>  }
>  
> -static void mxf_write_d10_video_packet(AVFormatContext *s, AVStream *st, 
> AVPacket *pkt)
> +static int mxf_write_d10_video_packet(AVFormatContext *s, AVStream *st, 
> AVPacket *pkt)
>  {
>  MXFContext *mxf = s->priv_data;
>  AVIOContext *pb = s->pb;
> @@ -2286,9 +2286,12 @@ static void mxf_write_d10_video_packet(AVFormatContext 
> *s, AVStream *st, AVPacke
>  ffio_fill(s->pb, 0, pad);
>  av_assert1(!(avio_tell(s->pb) & (KAG_SIZE-1)));
>  } else {
> -av_log(s, AV_LOG_WARNING, "cannot fill d-10 video packet\n");
> +av_log(s, AV_LOG_ERROR, "cannot fill d-10 video packet\n");
>  ffio_fill(s->pb, 0, pad);
> +return AVERROR(EIO);
>  }
> +
> +return 0;
>  }

Is this really better than not writing anything?

/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/mxfdec: fix seeking before the first keyframe

[Tomas Härdin]

On Sat, 2015-11-28 at 02:56 +0100, Marton Balint wrote:
> Regression since 53f2ef2c4afb1d49a679dea9163cb0e4671f3117.
> Fixes ticket #5017.
> 
> Signed-off-by: Marton Balint 
> ---
>  libavformat/mxfdec.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 429f46a..926d2a3 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -3181,6 +3181,16 @@ static int mxf_read_seek(AVFormatContext *s, int 
> stream_index, int64_t sample_ti
>  sample_time = FFMAX(sample_time, 0);
>  
>  if (t->fake_index) {
> +/* The first frames may not be keyframes in presentation order, 
> so
> + * we have to advance the target to be able to find the first
> + * keyframe backwards... */
> +if (!(flags & AVSEEK_FLAG_ANY) &&
> +(flags & AVSEEK_FLAG_BACKWARD) &&
> +t->ptses[0] != AV_NOPTS_VALUE &&

Can the size of t->ptses ever be zero here?


> +sample_time < t->ptses[0] &&
> +(t->fake_index[t->ptses[0]].flags & AVINDEX_KEYFRAME))

Do t->ptses always point inside t->fake_index?

> +sample_time = t->ptses[0];
> +

Should be OK otherwise, since it only affects seeking and fixes a
regression. Ideally we should be doing something better for seeking, I
think..

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Several, actually, regarding the QuickTime palette issue in Matroska

[Tomas Härdin]

On Wed, 2015-12-16 at 20:30 +0100, Mats Peterson wrote:
> On 12/16/2015 08:27 PM, Mats Peterson wrote:
> > On 12/16/2015 08:26 PM, Kieran Kunhya wrote:
> >>> I have said before that I'm not going to dwell into the complexities
> >>> of Git,
> >>> since it feels about as wasteful as programming a GUI in my book. If you
> >>> can't accept the patches in their current format, so be it. Thanks
> >>> for your
> >>> time and understanding.
> >>
> >> We accept git patches here and that's the way people send patches in
> >> thousands of open source projects.
> >> I wouldn't file my taxes written on a post-it note and we wouldn't
> >> accept random patch formats.
> >>
> >> Kieran
> >
> > That's one bad analogy.
> >
> > Mats
> >
> 
> Listen, I shouldn't have to learn Git just to be able to contribute with 
> some patches. It's crazy. But alright then, suit yourself. I guess I'll 
> have to keep patching FFmpeg myself here in the meantime.

You're expecting people to take time away from their lives to review
some patches for a free project. How do you expect this to work if
you're not willing to follow the project's workflow? There's even a
quite reasonable guide on the website [1]. FFmpeg is a complicated
project - not following the established procedure puts needless mental
burden on people who will end up having to maintain your patches.

Besides, learning git pays dividends. Since it's decentralized you can
use it to keep track of projects locally without needing a server, which
is useful for all kinds of things (assuming you believe in version
control).

/Tomas

[1] http://ffmpeg.org/developer.html#Submitting-patches



signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mxfenc: Fix integer overflow in length computation

[Tomas Härdin]

On Wed, 2015-12-09 at 18:13 +0100, Michael Niedermayer wrote:
> From: Michael Niedermayer 
> 
> Fixes: CID1341577
> 
> Signed-off-by: Michael Niedermayer 
> ---
>  libavformat/mxfenc.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index e4e4272..043daff 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -1268,11 +1268,11 @@ static void mxf_write_package(AVFormatContext *s, 
> enum MXFMetadataSetType type,
>  user_comment_count = mxf_write_user_comments(s, s->metadata);
>  mxf_write_metadata_key(pb, 0x013600);
>  PRINT_KEY(s, "Material Package key", pb->buf_ptr - 16);
> -klv_encode_ber_length(pb, 92 + name_size + (16*track_count) + 
> (16*user_comment_count) + 12*mxf->store_user_comments);
> +klv_encode_ber_length(pb, 92 + name_size + (16*track_count) + 
> (16*user_comment_count) + 12LL*mxf->store_user_comments);
>  } else {
>  mxf_write_metadata_key(pb, 0x013700);
>  PRINT_KEY(s, "Source Package key", pb->buf_ptr - 16);
> -klv_encode_ber_length(pb, 112 + name_size + (16*track_count) + 
> 12*mxf->store_user_comments); // 20 bytes length for descriptor reference
> +klv_encode_ber_length(pb, 112 + name_size + (16*track_count) + 
> 12LL*mxf->store_user_comments); // 20 bytes length for descriptor reference
>  }

Obviously OK :)

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/mxfdec: Set width to actual coded_width for AVCI50

[Tomas Härdin]

On Tue, 2015-11-24 at 12:02 +0100, Carl Eugen Hoyos wrote:
> Hi!
> 
> Attached patch fixes ticket #5029.
> 
> Please comment, Carl Eugen

Looks simple enough, but (ab)using MXFCodecUL like that has a slight
stink to it. Can we perhaps rename the struct to something that signals
its more general use of mapping ULs to integers?

Another thing: don't we have some table like this for H.264 in MXF
already?

/Tomas


signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: check if source_package is NULL

[Tomas Härdin]

On Tue, 2016-05-31 at 23:02 +0200, Marton Balint wrote:
> Fixes ticket #5554.
> 
> Signed-off-by: Marton Balint 
> ---
>  libavformat/mxfdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index f8cf922..9bf676c 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1914,7 +1914,7 @@ static int
> mxf_parse_structural_metadata(MXFContext *mxf)
>  break;
>  }
>  }
> -if (!source_track || !component)
> +if (!source_track || !component || !source_package)
>  continue;
>  
>  if (!(source_track->sequence = mxf_resolve_strong_ref(mxf,
> _track->sequence_ref, Sequence))) {

Looks OK. Crashes are important

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv2] Revert "do not write f2 if not interlaced"

[Tomas Härdin]

On Fri, 2016-01-29 at 17:45 +0100, Sebastian Dröge wrote:
> From: Sebastian Dröge 
> 
> This reverts commit 8ed82d8174a666f80ab8834e3617cbe91ae740a9.
> 
> SMPTE S377-1-2009c defines in F.4.1 that the Video Line Map should
> always be an array with two 32 bit integers as elements. This is
> repeated in G.2.12 with actual examples for progressive content,
> where the second value would always be 0.
> 
> Additionally, the IRT MXF analyser also lists this as the only
> error in the MXF output from ffmpeg: https://mxf-analyser-cloud.irt.de
> ---
>  libavformat/mxfenc.c  | 10 +-
>  tests/ref/lavf/mxf|  6 +++---
>  tests/ref/lavf/mxf_opatom |  2 +-
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 35c67f7..6da8b10 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -1013,7 +1013,7 @@ static void mxf_write_cdci_common(AVFormatContext *s, 
> AVStream *st, const UID ke
>  int stored_height = (st->codec->height+15)/16*16;
>  int display_height;
>  int f1, f2;
> -unsigned desc_size = size+8+8+8+8+8+8+8+5+16+sc->interlaced*4+12+20+5;
> +unsigned desc_size = size+8+8+8+8+8+8+8+5+16+4+12+20+5;

Not strictly related to this patch but:
Can we do something about this? Other muxers (such as movenc) use a
combination of ftell() and fseek() to rewrite size fields like these.
Or perhaps mxfenc needs to be able to write to a pipe? In that case
seeking in a short buffer is allowed in avio IIRC

>  if (sc->interlaced && sc->field_dominance)
>  desc_size += 5;
>  if (sc->signal_standard)
> @@ -1081,12 +1081,12 @@ static void mxf_write_cdci_common(AVFormatContext *s, 
> AVStream *st, const UID ke
>  f1 *= 2;
>  }
>  
> -mxf_write_local_tag(pb, 12+sc->interlaced*4, 0x320D);
> -avio_wb32(pb, sc->interlaced ? 2 : 1);
> +
> +mxf_write_local_tag(pb, 16, 0x320D);
> +avio_wb32(pb, 2);
>  avio_wb32(pb, 4);
>  avio_wb32(pb, f1);
> -if (sc->interlaced)
> -avio_wb32(pb, f2);
> +avio_wb32(pb, f2);

Looks OK..

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 09/12] avformat/mxfenc: use ff_parse_creation_time_metadata

[Tomas Härdin]

On Sat, 2016-02-06 at 20:13 +0100, Marton Balint wrote:
> Signed-off-by: Marton Balint 
> ---
>  libavformat/mxfenc.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 6da8b10..cd13f89 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2041,7 +2041,6 @@ static int mxf_write_header(AVFormatContext *s)
>  int i, ret;
>  uint8_t present[FF_ARRAY_ELEMS(mxf_essence_container_uls)] =
> {0};
>  const MXFSamplesPerFrame *spf = NULL;
> -AVDictionaryEntry *t;
>  int64_t timestamp = 0;
>  
>  if (!s->nb_streams)
> @@ -2212,9 +2211,7 @@ static int mxf_write_header(AVFormatContext *s)
>  sc->order = AV_RB32(sc->track_essence_element_key+12);
>  }
>  
> -if (t = av_dict_get(s->metadata, "creation_time", NULL, 0))
> -timestamp = ff_iso8601_to_unix_time(t->value);
> -if (timestamp)
> +if (ff_parse_creation_time_metadata(s, , 1) > 0)
>  mxf->timestamp = mxf_parse_timestamp(timestamp);
>  mxf->duration = -1;

Looks OK

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/mxfdec: Fix canopus essence element size

[Tomas Härdin]

On Thu, 2016-03-10 at 15:08 +0100, Carl Eugen Hoyos wrote:
> Hi!
> 
> While debugging ticket #5312, I realized that I included the 
> track number in the Canopus essence element.


OK

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [RFC]lavf/mxfdec: Assume first track if track number is unknown

[Tomas Härdin]

On Thu, 2016-03-10 at 15:06 +0100, Carl Eugen Hoyos wrote:
> Hi!
> 
> Attached patch fixes ticket #5312 here.
> The OP claims that the file plays fine with Mainconcept softare, 
> afaict the track number of the video track in the mxf header 
> (0x15010800) does not match the track number of the video frames 
> (0x15010500 == 352388352).

Hrm, this sounds suspicious. But I think happens for some other files
too, and I guess playing something is better than nothing.. Perhaps we
can have it be a bit more stringent with when it applies this hack?
Like if there's only a single track

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/mxfdec: Support canopus codecs

[Tomas Härdin]

On Wed, 2016-03-09 at 15:21 +0100, Carl Eugen Hoyos wrote:
> Hi!
> 
> Attached patch fixes ticket #5316 here.
> I unfortunately have no idea what the hunk in mxf_read_header() 
> does, decoding works fine without it.

Strange. Why was mxf_canopus_essence_element_key added then? Pushing
without it is fine if it fixes the issue, even preferable (since it'd
be simpler).

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 5/6] libavformat/mxf: add dnxhr codec ul

[Tomas Härdin]

On Mon, 2016-07-04 at 18:07 -0700, Mark Reid wrote:
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 0affca9..8f2f10a 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1098,6 +1098,10 @@ static int mxf_match_uid(const UID key, const UID uid, 
> int len)
>  static const MXFCodecUL *mxf_get_codec_ul(const MXFCodecUL *uls, UID *uid)
>  {
>  while (uls->uid[0]) {
> +/* match version byte for dnxhr */
> +if (uls->id == AV_CODEC_ID_DNXHR && !memcmp(uls->uid, *uid, 
> uls->matching_len))
> +break;
> +

Looks OK

I wonder if other codecs need this kind of special treatment. The
version byte isn't supposed to matter.. If any more pop up then we
could generalize this - for now this is good enough I suppose

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] New MXF maintainer wanted

[Tomas Härdin]

Hi

Long story short: I no longer work with MXF (or broadcast in general),
and my free time is currently being consumed by other projects.
Therefore I feel it'd be better if someone took over the role as MXF
maintainer.

I took a few shots at the recent MXF tickets on trac, but I've
forgotten too much to effectively fix them. Hence this mail

I can stay on as lxfdec maintainer, since I'm subscribed to the "lxf"
tag on trac. I will likely unsubscribe from this list, posting only if
there's an LXF ticket to deal with.

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [WIP] libcodec2 wrapper + de/muxer in FFmpeg

[Tomas Härdin]

 kbits/s\n");
 
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 74de41ab0f..f5531ab3f1 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -59,6 +59,7 @@ OBJS-$(CONFIG_AUDIODSP)+= audiodsp.o
 OBJS-$(CONFIG_BLOCKDSP)+= blockdsp.o
 OBJS-$(CONFIG_BSWAPDSP)+= bswapdsp.o
 OBJS-$(CONFIG_CABAC)   += cabac.o
+OBJS-$(CONFIG_CODEC2UTILS) += codec2utils.o
 OBJS-$(CONFIG_CRYSTALHD)   += crystalhd.o
 OBJS-$(CONFIG_DCT) += dct.o dct32_fixed.o dct32_float.o
 OBJS-$(CONFIG_ERROR_RESILIENCE)+= error_resilience.o
@@ -885,6 +886,8 @@ OBJS-$(CONFIG_ILBC_AT_ENCODER)+= audiotoolboxenc.o
 OBJS-$(CONFIG_PCM_ALAW_AT_ENCODER)+= audiotoolboxenc.o
 OBJS-$(CONFIG_PCM_MULAW_AT_ENCODER)   += audiotoolboxenc.o
 OBJS-$(CONFIG_LIBCELT_DECODER)+= libcelt_dec.o
+OBJS-$(CONFIG_LIBCODEC2_DECODER)  += libcodec2.o
+OBJS-$(CONFIG_LIBCODEC2_ENCODER)  += libcodec2.o
 OBJS-$(CONFIG_LIBFDK_AAC_DECODER) += libfdk-aacdec.o
 OBJS-$(CONFIG_LIBFDK_AAC_ENCODER) += libfdk-aacenc.o
 OBJS-$(CONFIG_LIBGSM_DECODER) += libgsmdec.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index 4712592a5f..5296fac507 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -618,6 +618,7 @@ static void register_all(void)
 REGISTER_DECODER(QDMC_AT,   qdmc_at);
 REGISTER_DECODER(QDM2_AT,   qdm2_at);
 REGISTER_DECODER(LIBCELT,   libcelt);
+REGISTER_ENCDEC (LIBCODEC2, libcodec2);
 REGISTER_ENCDEC (LIBFDK_AAC,libfdk_aac);
 REGISTER_ENCDEC (LIBGSM,libgsm);
 REGISTER_ENCDEC (LIBGSM_MS, libgsm_ms);
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index c594993766..488eb8b1f5 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -622,6 +622,7 @@ enum AVCodecID {
 AV_CODEC_ID_PAF_AUDIO,
 AV_CODEC_ID_ON2AVC,
 AV_CODEC_ID_DSS_SP,
+AV_CODEC_ID_CODEC2,
 
 AV_CODEC_ID_FFWAVESYNTH = 0x15800,
 AV_CODEC_ID_SONIC,
diff --git a/libavcodec/codec2utils.c b/libavcodec/codec2utils.c
new file mode 100644
index 00..8f5012f845
--- /dev/null
+++ b/libavcodec/codec2utils.c
@@ -0,0 +1,118 @@
+/*
+ * codec2 utility functions
+ * Copyright (c) 2017 Tomas Härdin
+ *
+ * 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 
+#include "internal.h"
+#include "libavcodec/codec2utils.h"
+
+//from codec2.h, repeated here to avoid a dependency
+#define CODEC2_MODE_3200 0
+#define CODEC2_MODE_2400 1
+#define CODEC2_MODE_1600 2
+#define CODEC2_MODE_1400 3
+#define CODEC2_MODE_1300 4
+#define CODEC2_MODE_1200 5
+#define CODEC2_MODE_700  6
+#define CODEC2_MODE_700B 7
+#define CODEC2_MODE_700C 8
+
+int avpriv_codec2_mode_from_str(void *logctx, const char *modestr) {
+//statically assert the size of avpriv_codec2_header
+//putting it here because all codec2 things depend on codec2utils
+switch(0) {
+case 0:
+case sizeof(avpriv_codec2_header) == 7: //if false then the compiler will complain
+break;
+}
+
+if (!modestr) {
+av_log(logctx, AV_LOG_ERROR, "raw codec2 streams need -mode set\n");
+return AVERROR(EINVAL);
+}
+
+#define MATCH(x) do { if (!strcmp(modestr, #x)) { return CODEC2_MODE_##x; } } while (0)
+MATCH(3200);
+MATCH(2400);
+MATCH(1600);
+MATCH(1400);
+MATCH(1300);
+MATCH(1200);
+MATCH(700);
+MATCH(700B);
+MATCH(700C);
+
+av_log(logctx, AV_LOG_ERROR, "invalid codec2 mode: %s\n", modestr);
+return AVERROR(EINVAL);
+}
+
+int avpriv_codec2_mode_bit_rate(void *logctx, int mode) {
+int ret = 8 * 8000 * avpriv_codec2_mode_block_align(logctx, mode) / avpriv_codec2_mode_frame_size(logctx, mode);
+if (ret <= 0) {
+av_log(logctx, AV_LOG_WARNING, "unknown codec2 mode %i, can't estimate bitrate\n", mode);
+}
+return ret;
+}
+
+int avpriv_codec2_mode_frame_size(void *logctx, int mode) {
+switch (mode) {
+case CODEC2_MODE_3200: return 160;
+case CODEC2_MODE_2400: return 160;
+case CODEC2_MODE_1600: return 320;
+case CODEC2_MODE_1400: return 320

Re: [FFmpeg-devel] [WIP] libcodec2 wrapper + de/muxer in FFmpeg

[Tomas Härdin]

On Thu, 2017-08-03 at 01:07 +0200, Carl Eugen Hoyos wrote:
> 2017-08-03 0:40 GMT+02:00 Tomas Härdin <tjop...@acc.umu.se>:
> 
> > 
> > Decoding a .c2 file is straightforward however, thanks to the
> > header:
> > 
> >   ffmpeg -i report2074.c2 out.wav
> The probe score is too high.
> I suspect you should also check for the major version,
> the score should be MAX/2 if the first 32bit are ok,
> significantly less for 24bit.

OK, didn't know that. Major being bumped would imply a new API, and
we're not likely to link two versions of the same library. I suspect it
would primarily impact the decoder, not the demuxer. But checking major
== 0 makes sense for now. libcodec2 does not yet expose version in its
headers, but should soon since I pointed that out to them :)

/Tomas



signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [WIP] libcodec2 wrapper + de/muxer in FFmpeg

[Tomas Härdin]

On Thu, 2017-08-03 at 13:00 +0200, Nicolas George wrote:
> Le sextidi 16 thermidor, an CCXXV, Tomas Härdin a écrit :
> > With both the raw demuxer and the encoder you need to specify the
> > desired mode, like this:
> The encoder could default to one the two.

Default on-the-air is 1300 (aka FreeDV 1600), so that seems like a
reasonable default

> > Some remarks:
> > 
> > * I had to make the ffmpeg CLI not complain about the 700 modes,
> > since
> > it thinks encoding at below 1 kbps is a user error
> It is just a warning. I am not really a fan of a special case like
> that,
> and it would be better split into a separate patch I think.

There's other codec-specific special cases in ffmpeg.c, like the one
for AV_CODEC_ID_VP9 which doesn't even have an explanation. Separate
patch is a good idea however.

> > * I have duplicated some minor functions in libcodec2 in
> > libavcodec/codec2utils.*. This avoid having to link libcodec2 just
> > for
> > remuxing, and in case someone writes a native decoder for
> > libavcodec
> The license allows it, but you neglected to give copyright
> attribution.

Alright, I can fix that. Although the only thing lifted straight are
the mode #defines

> > * Not sure if codec2utils should go in libavutil, libavcodec felt
> > good
> > enough
> Definitely libavcodec.

Cool.

> > 
> > * The demuxer tries to read up to 0.1 seconds worth of frames to
> > speed
> > things up a little without making seeking too broken. 3 frames = 12
> > bytes for the 700 bit/s modes, which decode to 960 samples
> I do not like the sound of that, but I will see the code.
> 
> > 
> > * The decoder is able to deal with multiple frames at a time, the
> > encoder does not need to
> ???

The encoder is given frame_size number of samples each time. Are there
times where this is not the case? I haven't set
AV_CODEC_CAP_SMALL_LAST_FRAME.

> > Feel free to bikeshed around whether extradata should be the entire
> > .c2
> > header or just the mode byte. It really only matters if we go with
> > RIFF
> > or ISOM mappings (.wav .avi .mov and friends), which I decided to
> > leave
> > out for now.
> It matters for inclusion in any format: Matroska, Nut, etc. Is
> anybody
> considering normalization?

I raised the possibility on [Freetel-codec2], either by coding mode as
part of the TwoCC (0xC208 for mode 8 for examples) or via extradata. I
imagine version + mode + flags in extradata would not be unreasonable.

> > +libcodec2_decoder_deps="libcodec2"
> > +libcodec2_decoder_select="codec2utils"
> > +libcodec2_encoder_deps="libcodec2"
> > +libcodec2_encoder_select="codec2utils"
> This and the similar hunks are not necessary, see below the comments
> about the Makefile.

I presume you mean codec2utils and not the libcodec2 deps.

> > +OBJS-$(CONFIG_CODEC2UTILS) += codec2utils.o
> You do not need a separate configuration option, you can just depend
> on
> the actual visible option:
> 
> +OBJS-$(CONFIG_CODEC2_DEMUXER)   += codec2utils.o
> +OBJS-$(CONFIG_CODEC2_MUXER) += codec2utils.o
> 
> Having the same file several times will not cause it to be included
> several times.
> 
> > +OBJS-$(CONFIG_LIBCODEC2_DECODER)  += libcodec2.o
> > +OBJS-$(CONFIG_LIBCODEC2_ENCODER)  += libcodec2.o
> +OBJS-$(CONFIG_LIBCODEC2_DECODER)  += libcodec2.o
> codec2utils.o
> +OBJS-$(CONFIG_LIBCODEC2_ENCODER)  += libcodec2.o
> codec2utils.o

Fixed.

> > +++ b/libavcodec/codec2utils.c
> > @@ -0,0 +1,118 @@
> > +/*
> > + * codec2 utility functions
> > 
> > + * Copyright (c) 2017 Tomas Härdin
> Missing copyright attribution if the imported code is non-trivial
> (better err on the side of more attribution).

Guess I'll nick the comment from the relevant files in libcodec2.


> > 
> > +int avpriv_codec2_mode_from_str(void *logctx, const char *modestr)
> > {
> Please normalize the coding style. Same below.

I presume you mean putting the function opening braces on their own
lines like the rest of FFmpeg. Fixed.

> > 
> > +//statically assert the size of avpriv_codec2_header
> > +//putting it here because all codec2 things depend on
> > codec2utils
> > +switch(0) {
> > +case 0:
> > +case sizeof(avpriv_codec2_header) == 7: //if false then the
> > compiler will complain
> > +break;
> > +}
> I see how it works. This is a nice trick. Want to make it an official
> macro FF_ASSERT_STATIC()?

Sure. Could go in avutil.h or something. Maybe even inside a static
function with a fake name like:

Re: [FFmpeg-devel] [PATCH] avcodec/zmbv: Check decomp_size

[Tomas Härdin]

On 2017-08-16 05:04, Michael Niedermayer wrote:

Fixes: OOM
Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
  libavcodec/zmbv.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
index f126515bd1..861098a0f2 100644
--- a/libavcodec/zmbv.c
+++ b/libavcodec/zmbv.c
@@ -589,8 +589,12 @@ static av_cold int decode_init(AVCodecContext *avctx)
  // Needed if zlib unused or init aborted before inflateInit
  memset(>zstream, 0, sizeof(z_stream));
  
-c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);

+if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > avctx->max_pixels) 
{


Are width and height constrained somewhere? If both end up around 1<<32 
then the multiplication can overflow.



+av_log(avctx, AV_LOG_ERROR, "Internal buffer larger than 
max_pixels\n");
+return AVERROR_INVALIDDATA;
+}
  
+c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);


Use 255ULL and 64ULL maybe? It's almost conceivable that you could have 
a file constructed to be (1<<31 - 255)x1 that passes the max_pixels check


/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [WIP] libcodec2 wrapper + de/muxer in FFmpeg

[Tomas Härdin]

On 2017-08-15 21:15, Moritz Barsnick wrote:

On Tue, Aug 08, 2017 at 23:49:45 +0200, Tomas Härdin wrote:

Feel free to comment

Don't forget to mention #1959 in the commit message.


Huh, I didn't know there was a ticket for it. That audiobook guy popped 
up on [Freetel-codec2] too, and thus the container discussion was born. 
It's also part of the reason why a raw demuxer is needed. Guess I'll 
have to send him some e-mail too, maybe I can nip some format headaches 
in the bud


/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/zmbv: Check decomp_size

[Tomas Härdin]

On 2017-08-16 14:35, Michael Niedermayer wrote:

On Wed, Aug 16, 2017 at 09:48:43AM +0200, Tomas Härdin wrote:

On 2017-08-16 05:04, Michael Niedermayer wrote:

Fixes: OOM
Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc>
---
  libavcodec/zmbv.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
index f126515bd1..861098a0f2 100644
--- a/libavcodec/zmbv.c
+++ b/libavcodec/zmbv.c
@@ -589,8 +589,12 @@ static av_cold int decode_init(AVCodecContext *avctx)
  // Needed if zlib unused or init aborted before inflateInit
  memset(>zstream, 0, sizeof(z_stream));
-c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);
+if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > avctx->max_pixels) 
{

Are width and height constrained somewhere? If both end up around
1<<32 then the multiplication can overflow.

width and height are signed integers and checked in avcodec_open2()

I can replicate the check here if you think depending on a distant
check is too fragile.


Nah, so long as it's checked. Too bad C doesn't have range typed 
integers like Ada



+av_log(avctx, AV_LOG_ERROR, "Internal buffer larger than 
max_pixels\n");
+return AVERROR_INVALIDDATA;
+}
+c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);

Use 255ULL and 64ULL maybe? It's almost conceivable that you could
have a file constructed to be (1<<31 - 255)x1 that passes the
max_pixels check

such file could exist but avcodec_open2() will reject this currently.
I think adding an explicit check of some kind would make sense
255ULL and 64ULL wont help as  decomp_size is just a unsigned int and
its passed into zlib which uses  a uInt ...

ill send a new patch

thx


Right, it might pass max_pixels but then overflow uInt..

/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/zmbv: Check decomp_size

[Tomas Härdin]

On 2017-08-16 16:03, Michael Niedermayer wrote:

Fixes: OOM
Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
  libavcodec/zmbv.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
index f126515bd1..b09dc41ebd 100644
--- a/libavcodec/zmbv.c
+++ b/libavcodec/zmbv.c
@@ -589,6 +589,11 @@ static av_cold int decode_init(AVCodecContext *avctx)
  // Needed if zlib unused or init aborted before inflateInit
  memset(>zstream, 0, sizeof(z_stream));
  
+if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > FFMIN(avctx->max_pixels, INT_MAX / 4) ) {

+av_log(avctx, AV_LOG_ERROR, "Internal buffer (decomp_size) larger than 
max_pixels or too large\n");
+return AVERROR_INVALIDDATA;
+}
+


Looks like a decent solution

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [WIP] libcodec2 wrapper + de/muxer in FFmpeg

[Tomas Härdin]

On Thu, 2017-08-03 at 22:24 +0200, Reimar Döffinger wrote:
> On Thu, Aug 03, 2017 at 12:40:04AM +0200, Tomas Härdin wrote:
> 
> > 
> > +//statically assert the size of avpriv_codec2_header
> > +//putting it here because all codec2 things depend on
> > codec2utils
> > +switch(0) {
> > +case 0:
> > +case sizeof(avpriv_codec2_header) == 7: //if false then the
> > compiler will complain
> > +break;
> > +}
> I was pretty sure we had some kind of static assert already,
> based on negative array size in a struct (so it can work outside
> functions)...
> But doesn't matter due to next comments...
> 
> > 
> > +*((avpriv_codec2_header*)avctx->extradata) =
> > avpriv_codec2_make_header(mode);
> I am pretty sure this is a strict aliasing violation.
> Even ignoring the ugliness and performance issues with
> returning structs and assigning them causing lots of
> pointless copies.
> Also, since all struct elements are bytes, it isn't
> really any more readable than just assigning to the
> byte array, with the names you not have in the struct
> in comments instead.
> And that is without going into all the ways someone could
> change that struct (e.g. in case of new features) that
> would completely break this kind of code due to endianness,
> alignment, ...
> Even just supporting both the current and a potential future version
> that changes any of the fields would be hard with this kind of code.

Aliasing violation is a good point, but it also doesn't matter here.
extradata isn't used anywhere else in the function.

The struct is mostly a handy way of getting names for all the bytes, so
a enum with offsets could work just as well:

enum avpriv_codec2_header_bytes {
  AVPRIV_CODEC2_MAGIC0 = 0,
  AVPRIV_CODEC2_MAGIC1,
  AVPRIV_CODEC2_MAGIC2,
  AVPRIV_CODEC2_VERSION_MAJOR,
  ...
};

Endianness is also not an issue since it's all bytes. But: one nice way
to do this is perhaps a pair of functions for parsing / creating the
struct out of the byte array and vice versa.

> > 
> > +if (avctx->extradata_size != sizeof(avpriv_codec2_header))
> > {
> > +av_log(avctx, AV_LOG_ERROR, "must have exactly %zu
> > bytes of extradata (got %i)\n",
> > +   sizeof(avpriv_codec2_header), avctx-
> > >extradata_size);
> > +}
> I would think at least if it is less you wouldn't want to just
> continue? Even if extradata is required to be padded (is it?),
> blindly selecting 0 as mode doesn't seem very likely to be right.

This has been fixed in the updated patchset

> > 
> > +output = (int16_t *)frame->data[0];
> The codec2 documentation seems pretty non-existant, so I
> assume you are right that this is always in native endianness.
> However from what I can see codec2 actually uses the "short" type,
> not int16_t.
> While it shouldn't make a difference in practice, if casting anyway
> I'd suggest using the type matching the API.

Some kind of static assert for sizeof(short) == sizeof(int16_t) would
be nicest I think. If someone's on a platform with non-16-bit shorts
and they want --enable-libcodec2 to work then they can probably write
and submit a patch.

> > 
> > +if (nframes > 0) {
> > +*got_frame_ptr = 1;
> > +}
> Not just *got_frame_ptr = nframes > 0; ?

Sure.

> > 
> > +int16_t *samples = (int16_t *)frame->data[0];
> You are casting the const away.
> Did they just forget to add the const to the API?
> If so, can you suggest it to be added?
> Otherwise if it's intentional you need to make a copy.

No const in the API, but adding it is of course easy enough. That won't
get into Debian for some time however, so users would have to build
their own libcodec2.

Version #defines in  could help with this, currently
there are none so that's one way to detect the non-const API. I'll have
to run that past the other guys.

> > +int ret;
> > +
> > +if ((ret = ff_alloc_packet2(avctx, avpkt, avctx->block_align,
> > 0)) < 0) {
> If you merge the declaration and assignment it you would get
> for free not having the assignment inside the if, which I
> still think is just horrible style. :)

Fair enough. I use this style in a few places when stacking a bunch of
inits and checks for ENOMEM and such. doc/developer.html doesn't seem
to have a preference, and I've seen the style used elsewhere in FFmpeg.
I went with splitting the assignments and testing in all the ifs in the
updated patchset

> > 
> > +//file starts wih 0xC0DEC2
> > +if (p->buf[0] == 0xC0 && p->buf[1] == 0xDE && p->buf[2] ==
> > 0xC2) {
> > +return AVPROBE_S

Re: [FFmpeg-devel] [WIP] libcodec2 wrapper + de/muxer in FFmpeg

[Tomas Härdin]

On Fri, 2017-08-04 at 19:20 +0200, Tomas Härdin wrote:
> TODO:
> 
> * have -mode be an integer and use the CONST system Nicolas mentioned
> * option for demuxing multiple frames at a time
> * sort the extradata aliasing thing
> * address API issues in libcodec2, possibly modify the format (there
> should still be time)

Had some time today and went over and addressed all TODOs but the API
issue in libcodec2. For that I need to figure out why reCAPTCHA is
broken on sourceforge.net, to be able to fix my ML registration >:|

I split the patchset into one for lavc and a second for lavf, plus a
third for the small ffmpeg CLI fix. Should make review simpler.

I also removed the 0xC0DEC2 magic from extradata, mostly as part of
reworking codec2utils

Finally I made codec2_probe only award AVPROBE_SCORE_MAX if the file
extension is .c2

Feel free to comment

/Tomas
From b693b6175289e6ad0c643462d8f69f6830086099 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <tjop...@acc.umu.se>
Date: Thu, 3 Aug 2017 17:33:04 +0200
Subject: [PATCH 3/3] Don't complain about codec2's 700 bit/s modes in ffmpeg.c

---
 ffmpeg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ffmpeg.c b/ffmpeg.c
index 888d19a647..09a5b541c0 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -3480,7 +3480,8 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len)
 av_buffersink_set_frame_size(ost->filter->filter,
 ost->enc_ctx->frame_size);
 assert_avoptions(ost->encoder_opts);
-if (ost->enc_ctx->bit_rate && ost->enc_ctx->bit_rate < 1000)
+if (ost->enc_ctx->bit_rate && ost->enc_ctx->bit_rate < 1000 &&
+ost->enc_ctx->codec_id != AV_CODEC_ID_CODEC2 /* don't complain about 700 bit/s modes */)
 av_log(NULL, AV_LOG_WARNING, "The bitrate parameter is set too low."
  " It takes bits/s as argument, not kbits/s\n");
 
-- 
2.13.3

From 057b941e82aeb8ccc835fb6a11baac4cb245a531 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <tjop...@acc.umu.se>
Date: Tue, 8 Aug 2017 15:28:06 +0200
Subject: [PATCH 2/3] Add muxer/demuxer for raw codec2 and .c2 files

---
 Changelog|   1 +
 doc/general.texi |   2 +
 libavformat/Makefile |   4 +
 libavformat/allformats.c |   2 +
 libavformat/codec2.c | 313 +++
 libavformat/rawenc.c |  13 ++
 libavformat/utils.c  |   1 +
 libavformat/version.h|   2 +-
 8 files changed, 337 insertions(+), 1 deletion(-)
 create mode 100644 libavformat/codec2.c

diff --git a/Changelog b/Changelog
index a3a16f0073..c95ffa8d16 100644
--- a/Changelog
+++ b/Changelog
@@ -33,6 +33,7 @@ version :
 - tlut2 video filter
 - floodfill video filter
 - codec2 en/decoding via libcodec2
+- muxer/demuxer for raw codec2 files and .c2 files
 
 version 3.3:
 - CrystalHD decoder moved to new decode API
diff --git a/doc/general.texi b/doc/general.texi
index fd8d657e4e..4bcc2b2d91 100644
--- a/doc/general.texi
+++ b/doc/general.texi
@@ -299,6 +299,8 @@ library:
 @item BRSTM @tab   @tab X
 @tab Audio format used on the Nintendo Wii.
 @item BWF   @tab X @tab X
+@item codec2 (raw)  @tab X @tab X
+@item codec2 (.c2 files)@tab X @tab X
 @item CRI ADX   @tab X @tab X
 @tab Audio-only format used in console video games.
 @item Discworld II BMV  @tab   @tab X
diff --git a/libavformat/Makefile b/libavformat/Makefile
index b0ef82cdd4..9aa3466161 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -128,6 +128,10 @@ OBJS-$(CONFIG_CAVSVIDEO_MUXER)   += rawenc.o
 OBJS-$(CONFIG_CDG_DEMUXER)   += cdg.o
 OBJS-$(CONFIG_CDXL_DEMUXER)  += cdxl.o
 OBJS-$(CONFIG_CINE_DEMUXER)  += cinedec.o
+OBJS-$(CONFIG_CODEC2_DEMUXER)+= ../libavcodec/codec2utils.o codec2.o rawdec.o pcm.o
+OBJS-$(CONFIG_CODEC2_MUXER)  += ../libavcodec/codec2utils.o codec2.o rawenc.o
+OBJS-$(CONFIG_CODEC2RAW_DEMUXER) += ../libavcodec/codec2utils.o codec2.o rawdec.o pcm.o
+OBJS-$(CONFIG_CODEC2RAW_MUXER)   += rawenc.o
 OBJS-$(CONFIG_CONCAT_DEMUXER)+= concatdec.o
 OBJS-$(CONFIG_CRC_MUXER) += crcenc.o
 OBJS-$(CONFIG_DATA_DEMUXER)  += rawdec.o
diff --git a/libavformat/allformats.c b/libavformat/allformats.c
index 1ebc14231c..26f0f1eccd 100644
--- a/libavformat/allformats.c
+++ b/libavformat/allformats.c
@@ -94,6 +94,8 @@ static void register_all(void)
 REGISTER_DEMUXER (CDG,  cdg);
 REGISTER_DEMUXER (CDXL, cdxl);
 REGISTER_DEMUXER (CINE, cine);
+REGISTER_MUXDEMUX(CODEC2,   codec2);
+REGISTER_MUXDEMUX(CODEC2RAW,codec

Re: [FFmpeg-devel] [PATCH 2/6] avcodec/vorbisenc: Apply dynamic frame lengths

[Tomas Härdin]

On 2017-08-22 03:23, Tyler Jones wrote:

+static int create_residues(vorbis_enc_context *venc)
+{
+int res, ret;
+vorbis_enc_residue *rc;
+
+venc->nresidues = 2;
+venc->residues  = av_malloc(sizeof(vorbis_enc_residue) * venc->nresidues);


av_malloc_array()? Applies to most av_malloc() in there



-// single mapping
-mc = >mappings[0];
-mc->submaps = 1;
-mc->mux = av_malloc(sizeof(int) * venc->channels);
-if (!mc->mux)
-return AVERROR(ENOMEM);
-for (i = 0; i < venc->channels; i++)
-mc->mux[i] = 0;
-mc->floor   = av_malloc(sizeof(int) * mc->submaps);
-mc->residue = av_malloc(sizeof(int) * mc->submaps);
-if (!mc->floor || !mc->residue)
-return AVERROR(ENOMEM);
-for (i = 0; i < mc->submaps; i++) {
-mc->floor[i]   = 0;
-mc->residue[i] = 0;
-}
-mc->coupling_steps = venc->channels == 2 ? 1 : 0;
-mc->magnitude  = av_malloc(sizeof(int) * mc->coupling_steps);
-mc->angle  = av_malloc(sizeof(int) * mc->coupling_steps);
-if (!mc->magnitude || !mc->angle)
-return AVERROR(ENOMEM);
-if (mc->coupling_steps) {
-mc->magnitude[0] = 0;
-mc->angle[0] = 1;
+for (map = 0; map < venc->nmappings; map++) {
+mc = >mappings[map];
+mc->submaps = 1;
+mc->mux = av_malloc(sizeof(int) * venc->channels);
+if (!mc->mux)
+return AVERROR(ENOMEM);
+for (i = 0; i < venc->channels; i++)
+mc->mux[i] = 0;
+mc->floor   = av_malloc(sizeof(int) * mc->submaps);
+mc->residue = av_malloc(sizeof(int) * mc->submaps);
+if (!mc->floor || !mc->residue)
+return AVERROR(ENOMEM);
+for (i = 0; i < mc->submaps; i++) {
+mc->floor[i]   = map;
+mc->residue[i] = map;
+}
+mc->coupling_steps = venc->channels == 2 ? 1 : 0;
+mc->magnitude  = av_malloc(sizeof(int) * mc->coupling_steps);
+mc->angle  = av_malloc(sizeof(int) * mc->coupling_steps);
+if (!mc->magnitude || !mc->angle)
+return AVERROR(ENOMEM);
+if (mc->coupling_steps) {
+mc->magnitude[0] = 0;
+mc->angle[0] = 1;
+}
  }


Maybe nitpicking, but it would be clearer what the changes are if you 
put the indentation change in a separate commit




-move_audio(venc, avctx->frame_size);
+if (venc->transient < 0) {
+move_audio(venc, avctx->frame_size);
  
-for (ch = 0; ch < venc->channels; ch++) {

-float *scratch = venc->scratch + 2 * ch * frame_size + frame_size;
+for (ch = 0; ch < venc->channels; ch++) {
+float *scratch = venc->scratch + 2 * ch * long_win + long_win;
  
-if (!ff_psy_vorbis_block_frame(>vpctx, scratch, ch,

-   frame_size, block_size))
-curr_win = 0;
+if (!ff_psy_vorbis_block_frame(>vpctx, scratch, ch,
+   long_win, short_win))
+next_win = 0;
+}
  }


Same here

/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Fix signed integer overflows

[Tomas Härdin]

On 2017-08-18 08:14, Vitaly Buka wrote:

Signed integer overflow is undefined behavior.
Detected with clang and -fsanitize=signed-integer-overflow

Signed-off-by: Vitaly Buka 
---
  libavcodec/utils.c| 2 +-
  libavformat/aviobuf.c | 4 +++-
  libavformat/mov.c | 2 +-
  3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 1336e921c9..024dc1f3e2 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -971,7 +971,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
  }
  
  if (!avctx->rc_initial_buffer_occupancy)

-avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3 / 4;
+avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3ll / 
4;


Didn't know you could use lower case for long long constants. Neat


  if (avctx->ticks_per_frame && avctx->time_base.num &&
  avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) {
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 7f4e740a33..319a402faf 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -259,7 +259,9 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int 
whence)
  offset1 = pos + (s->buf_ptr - s->buffer);
  if (offset == 0)
  return offset1;
-offset += offset1;
+// Use unsigned type to avoid undefined behavior of singed overflow.
+// Code below will report error on overflow anyway.
+offset += (uint64_t)offset1;


I presume offset1 is some value which is never >= 1<<63?


  }
  if (offset < 0)
  return AVERROR(EINVAL);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 522ce60c2d..a14c9f182b 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -5572,7 +5572,7 @@ static int mov_read_default(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
  
  if (atom.size < 0)

  atom.size = INT64_MAX;
-while (total_size + 8 <= atom.size && !avio_feof(pb)) {
+while (total_size <= atom.size - 8 && !avio_feof(pb)) {


atom.size can never be < 8?

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] License consent (cinepakenc.c)

[Tomas Härdin]

Hi

I got a question from Diego via IRC about what license cinepakenc.c
actually has, since the first commit message just has a message by"Rl"
saying that I said it was OK but with no appropriately signed message
from me. So I'm posting this just to clarify things:

I consent to the original license placed on cinepakenc.c on Wed Jan 22
11:12:11 2014 +0100 (commit 59dbc36f49db5cfd9d2ad4b00ef2e3336173ee8d).

Just for fun I gave the encoder a try, and the output plays just fine
on Windows 3.1 in dosbox-x :)

My e-mail address changed a while back since I've changed employer, so
MAINTAINERS should be updated to point to this one. I also don't work
with MXF any longer, so a new maintainer may be needed for that

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] License consent (cinepakenc.c)

[Tomas Härdin]

On Tue, 2017-05-16 at 15:42 -0300, James Almer wrote:
> On 5/16/2017 3:13 PM, Tomas Härdin wrote:
> > 
> > Hi
> > 
> > I got a question from Diego via IRC about what license cinepakenc.c
> > actually has, since the first commit message just has a message
> > by"Rl"
> > saying that I said it was OK but with no appropriately signed
> > message
> > from me. So I'm posting this just to clarify things:
> > 
> > I consent to the original license placed on cinepakenc.c on Wed Jan
> > 22
> > 11:12:11 2014 +0100 (commit
> > 59dbc36f49db5cfd9d2ad4b00ef2e3336173ee8d).
> > 
> > Just for fun I gave the encoder a try, and the output plays just
> > fine
> > on Windows 3.1 in dosbox-x :)
> Nice, i guess :p
> 
> > 
> > 
> > My e-mail address changed a while back since I've changed employer,
> > so
> > MAINTAINERS should be updated to point to this one. I also don't
> > work
> > with MXF any longer, so a new maintainer may be needed for that
> MAINTAINERS doesn't list email addresses, just GPG fingerprints, and
> the
> one listed for you matches the one you're still using today.
> If you have push access you can remove the mxfdec mention, otherwise
> i
> can do that for you.

Sure. I see I'm listed for lxfdec too, but I think I can still manage
that. And the cinepak encoder has Rl listed so that's covered

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mxfenc: fix aspect ratio when writing 16:9 DV frames

[Tomas Härdin]

On 2017-09-14 15:44, Tobias Rapp wrote:

Signed-off-by: Tobias Rapp 
---
  libavformat/mxfenc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 7289e0b..da4d7b4 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -1810,7 +1810,7 @@ static int mxf_parse_dv_frame(AVFormatContext *s, 
AVStream *st, AVPacket *pkt)
  stype= vs_pack[3] & 0x1f;
  pal  = (vs_pack[3] >> 5) & 0x1;
  
-if ((vs_pack[2] & 0x07) == 0x02)

+if ((vsc_pack[2] & 0x07) == 0x02)
  sc->aspect_ratio = (AVRational){ 16, 9 };
  else
  sc->aspect_ratio = (AVRational){ 4, 3 };


Might want to add some { } around those two cases while you're at it

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfdec: use the common packet pts setter function for opatom files

[Tomas Härdin]

On 2017-09-13 21:31, Marton Balint wrote:


On Fri, 8 Sep 2017, Michael Niedermayer wrote:


On Thu, Sep 07, 2017 at 05:11:40PM +0200, Marton Balint wrote:

Fixes ticket #6631.

Signed-off-by: Marton Balint 
---
 libavformat/mxfdec.c | 12 +++-
 1 file changed, 3 insertions(+), 9 deletions(-)


please add a fate this if you push this


Unfortunately we only have a sample which is around 16 MB but it seems 
like an overkill to add such a big sample to the fate suite for such a 
minor feature. And I am also not very fond of truncating the file and 
adding truncated files to the fate suite... So unless somebody can 
provide an opAtom AVC intra file which is only a few frames long, I 
see no good way to fate test this. Do you have an idea how to proceed?


Try having the user that reported the bug generate a smaller + lower res 
sample?


/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Tomas Härdin]

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

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] add Weston Capture demuxer, parser and decoder

[Tomas Härdin]

On Sun, 2017-09-10 at 21:03 +0200, Paul B Mahol wrote:
> +static av_cold int wcap_decode_init(AVCodecContext *avctx)
> +{
> +WCAPContext *s = avctx->priv_data;
> +uint32_t format;
> +
> +if (avctx->extradata && avctx->extradata_size >= 4) {
> +format = AV_RL32(avctx->extradata);
> +
> +switch (format) {
> +case 0x34325852: avctx->pix_fmt = AV_PIX_FMT_RGB0; break;
> +case 0x34325842: avctx->pix_fmt = AV_PIX_FMT_BGR0; break;
> +case 0x34325258: avctx->pix_fmt = AV_PIX_FMT_0RGB; break;
> +case 0x34324258: avctx->pix_fmt = AV_PIX_FMT_0BGR; break;
> +}

default: return AVERROR_INVALIDDATA ?

> +}
> +
> +s->frame = av_frame_alloc();
> +if (!s->frame)
> +return AVERROR(ENOMEM);
> +
> +return 0;
> +}
> +
> +static void clear(AVCodecContext *avctx)
> +{
> +WCAPContext *s = avctx->priv_data;
> +int y;
> +
> +if (!s->frame->buf[0])
> +return;
> +
> +for (y = 0; y < avctx->height; y++) {
> +memset(s->frame->data[0] + y * s->frame->linesize[0], 0,
> avctx->width * 4);
> +}
> +}

Wasn't there a AVFrame clear function added to lavu recently?

> +
> +static int wcap_decode_frame(AVCodecContext *avctx, void *data,
> +int *got_frame, AVPacket *avpkt)
> +{
> +WCAPContext *s = avctx->priv_data;
> +AVFrame *frame = s->frame;
> +uint32_t nrects, x1, y1, x2, y2;
> +int ret, n, i, k, x;
> +GetByteContext gb;
> +uint8_t *dst;
> +
> +if ((ret = av_image_check_size(avctx->width, avctx->height, 0,
> NULL)) < 0)
> +return ret;
> +
> +bytestream2_init(, avpkt->data, avpkt->size);
> +
> +if ((ret = ff_reget_buffer(avctx, frame)) < 0)
> +return ret;
> +
> +if (avpkt->flags & AV_PKT_FLAG_KEY) {
> +clear(avctx);
> +}
> +
> +bytestream2_skip(, 4);
> +nrects = bytestream2_get_le32();
> +
> +for (n = 0; n < nrects; n++) {

n is signed, nrects is unsigned -> infinite loop for any value >=
0x8000

Might want to bound nrecs <= avpkt->size / 20 too, or check if
the GetByteContext ran out of bits

> +x1 = bytestream2_get_le32();
> +y1 = bytestream2_get_le32();
> +x2 = bytestream2_get_le32();
> +y2 = bytestream2_get_le32();
> +
> +if (x1 >= x2 || y1 >= y2 || x2 > avctx->width || y2 > avctx-
> >height ||
> +(x2 - x1) > avctx->width || (y2 - y1) > avctx->height)
> +return AVERROR_INVALIDDATA;
> +
> +x = x1;
> +dst = frame->data[0] + (avctx->height - y1 - 1) * frame-
> >linesize[0];
> +
> +for (i = 0; i < (x2 - x1) * (y2 - y1);) {

Frames can't be larger than 2 Gi pixels, right?

> +unsigned v = bytestream2_get_le32();
> +int run_len = v >> 24;
> +
> +if (run_len < 0xE0)
> +run_len++;
> +else
> +run_len = 1 << (run_len - 0xE0 + 7);

This overflows on 32-bit if run_len >= 0xF8

(holy crap, C's lack of static checks is annoying)

> +
> +i += run_len;
> +for (k = 0; k < run_len; k++) {
> +dst[x*4 + 1] += v & 0xFF;
> +dst[x*4 + 2] += (v >> 8) & 0xFF;
> +dst[x*4 + 3] += (v >> 16) & 0xFF;
> +x++;
> +if (x == x2) {
> +x = x1;
> +dst -= frame->linesize[0];
> +}

This can easily write past the end of the frame


/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/4] avformat/mxfenc: write reel_name if metadata key is present

[Tomas Härdin]

On 2017-11-28 05:35, Mark Reid wrote:

On Mon, Nov 27, 2017 at 2:40 AM, Tomas Härdin <tjop...@acc.umu.se> wrote:


On Sun, 2017-11-26 at 21:42 -0800, Mark Reid wrote:

@@ -1396,13 +1410,17 @@ static int mxf_write_package(AVFormatContext
*s, MXFPackage *package)
  }

  // write multiple descriptor reference
-if (package->type == SourcePackage) {
+if (package->instance == 1) {

Would only ever SourcePackage have instance != 0? What if we have
multiple MaterialPackage?
Saying (package->type == SourcePackage && package->instance == 1) might
be appropriate


simple enough, I'll do that



  mxf_write_local_tag(pb, 16, 0x4701);
  if (s->nb_streams > 1) {
  mxf_write_uuid(pb, MultipleDescriptor, 0);
  mxf_write_multi_descriptor(s);
  } else
  mxf_write_uuid(pb, SubDescriptor, 0);
+} else if (package->instance == 2) {

Same here


+mxf_write_local_tag(pb, 16, 0x4701);
+mxf_write_uuid(pb, TapeDescriptor, 0);
+mxf_write_tape_descriptor(s);
  }

  // write timecode track
@@ -1416,7 +1434,7 @@ static int mxf_write_package(AVFormatContext
*s, MXFPackage *package)
  mxf_write_sequence(s, st, package);
  mxf_write_structural_component(s, st, package);

-if (package->type == SourcePackage) {
+if (package->instance == 1) {

And here


@@ -1474,6 +1494,26 @@ static int
mxf_write_header_metadata_sets(AVFormatContext *s)
  }
  }

+if (entry = av_dict_get(s->metadata, "reel_name", NULL, 0)) {

Parenthesis around this and maybe an equality check. Or move the
assignment outside the if.



Ok, I'll move it outside the if


+packages[2].name = entry->value;

+} else {
+/* check if any of the streams contain a reel_name */
+for (i = 0; i < s->nb_streams; i++) {
+st = s->streams[i];
+if (entry = av_dict_get(st->metadata, "reel_name", NULL,
0)) {
+packages[2].name = entry->value;
+break;

Is it possible to set more than one reel_name? Conflicting values
should probably result in an error (both s->metadata and st->metadata).



yes this is a bit messy,  mxfdec puts the reel_names on streams. Even if
the all the streams source the same reel package,  I'd like to try and fix
that in mxfdec and put them on format level. How about for mxfenc being
strict and only accepting reel_name metadata at the format level for now?


Yes, strictness is good. Can always be relaxed later, unlike the opposite

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/4] avformat/mxfenc: pass MXFPackage around instead of type

[Tomas Härdin]

On Tue, 2017-11-28 at 21:09 +0100, Michael Niedermayer wrote:
> On Mon, Nov 27, 2017 at 11:00:51AM +0100, Tomas Härdin wrote:
> > On Sun, 2017-11-26 at 21:42 -0800, Mark Reid wrote:
> > > ---
> > >  libavformat/mxfenc.c | 99 +-
> > > 
> > > --
> > >  1 file changed, 55 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > index 035e65ed43..ed6ecbf541 100644
> > > --- a/libavformat/mxfenc.c
> > > +++ b/libavformat/mxfenc.c
> > > @@ -101,6 +101,12 @@ typedef struct MXFContainerEssenceEntry {
> > >  void (*write_desc)(AVFormatContext *, AVStream *);
> > >  } MXFContainerEssenceEntry;
> > >  
> > > +typedef struct MXFPackage {
> > > +char *name;
> > > +enum MXFMetadataSetType type;
> > > +int instance;
> > > +} MXFPackage;
> > > [...]
> > 
> > Looks OK.
> 
> will apply
> 
> thanks

It sounded like he's working on an alternate patchset, see the PATCH
3/4 thread

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2 1/3] avformat/mxfenc: use track count to generate component instance uuid

[Tomas Härdin]

On 2017-11-29 05:11, Mark Reid wrote:

@@ -980,7 +980,7 @@ static void mxf_write_structural_component(AVFormatContext 
*s, AVStream *st, MXF
  
  // write uid

  mxf_write_local_tag(pb, 16, 0x3C0A);
-mxf_write_uuid(pb, package->type == MaterialPackage ? SourceClip: SourceClip 
+ TypeBottom, st->index);
+mxf_write_uuid(pb, SourceClip, mxf->track_uuid_offset);
  
  PRINT_KEY(s, "structural component uid", pb->buf_ptr - 16);

  mxf_write_common_fields(s, st);
@@ -1357,7 +1357,7 @@ static void mxf_write_package(AVFormatContext *s, 
MXFPackage *package)
  
  // write package umid

  mxf_write_local_tag(pb, 32, 0x4401);
-mxf_write_umid(s, package->type == SourcePackage);
+mxf_write_umid(s, package->instance);
  PRINT_KEY(s, "package umid second part", pb->buf_ptr - 16);
  
  // package name

@@ -1375,10 +1375,9 @@ static void mxf_write_package(AVFormatContext *s, 
MXFPackage *package)
  // write track refs
  mxf_write_local_tag(pb, track_count*16 + 8, 0x4403);
  mxf_write_refs_count(pb, track_count);
-mxf_write_uuid(pb, package->type == MaterialPackage ? Track :
-   Track + TypeBottom, -1); // timecode track
+mxf_write_uuid(pb, Track, mxf->track_uuid_offset); // timecode track
  for (i = 0; i < s->nb_streams; i++)
-mxf_write_uuid(pb, package->type == MaterialPackage ? Track : Track + 
TypeBottom, i);
+mxf_write_uuid(pb, Track,  mxf->track_uuid_offset + i + 1);


Do these refer to tracks that are about to be written? Seems so as best 
I can tell, so I guess it works. I'd be happier if the referencing was 
done more explicitly rather than implicitly


  
  // write user comment refs

  if (mxf->store_user_comments) {
@@ -1402,12 +1401,14 @@ static void mxf_write_package(AVFormatContext *s, 
MXFPackage *package)
  mxf_write_track(s, mxf->timecode_track, package);
  mxf_write_sequence(s, mxf->timecode_track, package);
  mxf_write_timecode_component(s, mxf->timecode_track, package);
+mxf->track_uuid_offset++;
  
  for (i = 0; i < s->nb_streams; i++) {

  AVStream *st = s->streams[i];
  mxf_write_track(s, st, package);
  mxf_write_sequence(s, st, package);
  mxf_write_structural_component(s, st, package);
+mxf->track_uuid_offset++;
  


/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2 2/3] avformat/mxfenc: write reel_name if metadata key is present

[Tomas Härdin]

On 2017-11-29 05:11, Mark Reid wrote:

---
  libavformat/mxf.h|  1 +
  libavformat/mxfenc.c | 42 +++---
  2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/libavformat/mxf.h b/libavformat/mxf.h
index 2d5b44943b..ffcc429a8b 100644
--- a/libavformat/mxf.h
+++ b/libavformat/mxf.h
@@ -47,6 +47,7 @@ enum MXFMetadataSetType {
  EssenceContainerData,
  EssenceGroup,
  TaggedValue,
+TapeDescriptor,
  };
  
  enum MXFFrameLayout {

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index baaeb8c617..02192aa22b 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -105,6 +105,7 @@ typedef struct MXFPackage {
  char *name;
  enum MXFMetadataSetType type;
  int instance;
+struct MXFPackage *ref;
  } MXFPackage;
  
  enum ULIndex {

@@ -991,20 +992,33 @@ static void 
mxf_write_structural_component(AVFormatContext *s, AVStream *st, MXF
  
  // write source package uid, end of the reference

  mxf_write_local_tag(pb, 32, 0x1101);
-if (package->type == SourcePackage) {
+if (!package->ref) {
  for (i = 0; i < 4; i++)
  avio_wb64(pb, 0);
  } else
-mxf_write_umid(s, 1);
+mxf_write_umid(s, package->ref->instance);
  
  // write source track id

  mxf_write_local_tag(pb, 4, 0x1102);
-if (package->type == SourcePackage)
+if (package->type == SourcePackage && !package->ref)
  avio_wb32(pb, 0);
  else
  avio_wb32(pb, st->index+2);
  }
  
+static void mxf_write_tape_descriptor(AVFormatContext *s)

+{
+AVIOContext *pb = s->pb;
+
+mxf_write_metadata_key(pb, 0x012e00);
+PRINT_KEY(s, "tape descriptor key", pb->buf_ptr - 16);
+klv_encode_ber_length(pb, 20);
+mxf_write_local_tag(pb, 16, 0x3C0A);
+mxf_write_uuid(pb, TapeDescriptor, 0);
+PRINT_KEY(s, "tape_desc uid", pb->buf_ptr - 16);
+}
+
+
  static void mxf_write_multi_descriptor(AVFormatContext *s)
  {
  MXFContext *mxf = s->priv_data;
@@ -1388,13 +1402,17 @@ static void mxf_write_package(AVFormatContext *s, 
MXFPackage *package)
  }
  
  // write multiple descriptor reference

-if (package->type == SourcePackage) {
+if (package->type == SourcePackage && package->instance == 1) {
  mxf_write_local_tag(pb, 16, 0x4701);
  if (s->nb_streams > 1) {
  mxf_write_uuid(pb, MultipleDescriptor, 0);
  mxf_write_multi_descriptor(s);
  } else
  mxf_write_uuid(pb, SubDescriptor, 0);
+} else if (package->type == SourcePackage && package->instance == 2) {
+mxf_write_local_tag(pb, 16, 0x4701);
+mxf_write_uuid(pb, TapeDescriptor, 0);
+mxf_write_tape_descriptor(s);
  }
  
  // write timecode track

@@ -1410,7 +1428,7 @@ static void mxf_write_package(AVFormatContext *s, 
MXFPackage *package)
  mxf_write_structural_component(s, st, package);
  mxf->track_uuid_offset++;
  
-if (package->type == SourcePackage) {

+if (package->type == SourcePackage && package->instance == 1) {
  MXFStreamContext *sc = st->priv_data;
  mxf_essence_container_uls[sc->index].write_desc(s, st);
  }
@@ -1445,12 +1463,13 @@ static int 
mxf_write_header_metadata_sets(AVFormatContext *s)
  AVDictionaryEntry *entry = NULL;
  AVStream *st = NULL;
  int i;
-
-MXFPackage packages[2] = {};
+MXFPackage packages[3] = {};
  int package_count = 2;
  packages[0].type = MaterialPackage;
  packages[1].type = SourcePackage;
  packages[1].instance = 1;
+packages[0].ref = [1];
+
  
  if (entry = av_dict_get(s->metadata, "material_package_name", NULL, 0))

 packages[0].name = entry->value;
@@ -1468,6 +1487,15 @@ static int 
mxf_write_header_metadata_sets(AVFormatContext *s)
  }
  }
  
+entry = av_dict_get(s->metadata, "reel_name", NULL, 0);

+if (entry) {
+packages[2].name = entry->value;
+packages[2].type = SourcePackage;
+packages[2].instance = 2;
+packages[1].ref = [2];
+package_count = 3;
+}
+
  mxf_write_preface(s);
  mxf_write_identification(s);
  mxf_write_content_storage(s, packages, package_count);


Looks OK

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2 1/3] avformat/mxfenc: use track count to generate component instance uuid

[Tomas Härdin]

On Wed, 2017-11-29 at 20:18 -0800, Mark Reid wrote:
> On Wed, Nov 29, 2017 at 1:36 AM, Tomas Härdin <tjop...@acc.umu.se>
> wrote:
> 
> > On 2017-11-29 05:11, Mark Reid wrote:
> > 
> > > @@ -980,7 +980,7 @@ static void
> > > mxf_write_structural_component(AVFormatContext
> > > *s, AVStream *st, MXF
> > > // write uid
> > >   mxf_write_local_tag(pb, 16, 0x3C0A);
> > > -mxf_write_uuid(pb, package->type == MaterialPackage ?
> > > SourceClip:
> > > SourceClip + TypeBottom, st->index);
> > > +mxf_write_uuid(pb, SourceClip, mxf->track_uuid_offset);
> > > PRINT_KEY(s, "structural component uid", pb->buf_ptr -
> > > 16);
> > >   mxf_write_common_fields(s, st);
> > > @@ -1357,7 +1357,7 @@ static void
> > > mxf_write_package(AVFormatContext *s,
> > > MXFPackage *package)
> > > // write package umid
> > >   mxf_write_local_tag(pb, 32, 0x4401);
> > > -mxf_write_umid(s, package->type == SourcePackage);
> > > +mxf_write_umid(s, package->instance);
> > >   PRINT_KEY(s, "package umid second part", pb->buf_ptr - 16);
> > > // package name
> > > @@ -1375,10 +1375,9 @@ static void
> > > mxf_write_package(AVFormatContext *s,
> > > MXFPackage *package)
> > >   // write track refs
> > >   mxf_write_local_tag(pb, track_count*16 + 8, 0x4403);
> > >   mxf_write_refs_count(pb, track_count);
> > > -mxf_write_uuid(pb, package->type == MaterialPackage ? Track
> > > :
> > > -   Track + TypeBottom, -1); // timecode track
> > > +mxf_write_uuid(pb, Track, mxf->track_uuid_offset); //
> > > timecode track
> > >   for (i = 0; i < s->nb_streams; i++)
> > > -mxf_write_uuid(pb, package->type == MaterialPackage ?
> > > Track :
> > > Track + TypeBottom, i);
> > > +mxf_write_uuid(pb, Track,  mxf->track_uuid_offset + i +
> > > 1);
> > > 
> > 
> > Do these refer to tracks that are about to be written? Seems so as
> > best I
> > can tell, so I guess it works. I'd be happier if the referencing
> > was done
> > more explicitly rather than implicitly
> 
> 
> yes they refer to the tracks about to be written. Those uuids need to
> match
> the uuid instance tag (0x3C0A) on a track written in mxf_write_track.
> then
> the uuid ref on the track needs to match the one on the sequence, and
> then
> the one the sequence needs to match the one on the component.
> 
> would you rather the instance id be a argument to each of the write
> functions? for example:
> mxf_write_package(AVFormatContext *s, MXFPackage *package, int
> track_uuid_offset)
> mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage
> *package, int
> instance_id)
> would that be more explicit?

I'm not quite sure tbh. It's been a few years since I worked with MXF,
so it's harder to say. Maybe a comment would be enough

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/4] avformat/mxfenc: write reel_name if metadata key is present

[Tomas Härdin]

On Sun, 2017-11-26 at 21:42 -0800, Mark Reid wrote:
> @@ -1396,13 +1410,17 @@ static int mxf_write_package(AVFormatContext
> *s, MXFPackage *package)
>  }
>  
>  // write multiple descriptor reference
> -if (package->type == SourcePackage) {
> +if (package->instance == 1) {

Would only ever SourcePackage have instance != 0? What if we have
multiple MaterialPackage?
Saying (package->type == SourcePackage && package->instance == 1) might
be appropriate

>  mxf_write_local_tag(pb, 16, 0x4701);
>  if (s->nb_streams > 1) {
>  mxf_write_uuid(pb, MultipleDescriptor, 0);
>  mxf_write_multi_descriptor(s);
>  } else
>  mxf_write_uuid(pb, SubDescriptor, 0);
> +} else if (package->instance == 2) {

Same here

> +mxf_write_local_tag(pb, 16, 0x4701);
> +mxf_write_uuid(pb, TapeDescriptor, 0);
> +mxf_write_tape_descriptor(s);
>  }
>  
>  // write timecode track
> @@ -1416,7 +1434,7 @@ static int mxf_write_package(AVFormatContext
> *s, MXFPackage *package)
>  mxf_write_sequence(s, st, package);
>  mxf_write_structural_component(s, st, package);
>  
> -if (package->type == SourcePackage) {
> +if (package->instance == 1) {

And here

> @@ -1474,6 +1494,26 @@ static int
> mxf_write_header_metadata_sets(AVFormatContext *s)
>  }
>  }
>  
> +if (entry = av_dict_get(s->metadata, "reel_name", NULL, 0)) {

Parenthesis around this and maybe an equality check. Or move the
assignment outside the if.

> +packages[2].name = entry->value;
> +} else {
> +/* check if any of the streams contain a reel_name */
> +for (i = 0; i < s->nb_streams; i++) {
> +st = s->streams[i];
> +if (entry = av_dict_get(st->metadata, "reel_name", NULL,
> 0)) {
> +packages[2].name = entry->value;
> +break;

Is it possible to set more than one reel_name? Conflicting values
should probably result in an error (both s->metadata and st->metadata).

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/4] avformat/mxfenc: pass MXFPackage around instead of type

[Tomas Härdin]

On Sun, 2017-11-26 at 21:42 -0800, Mark Reid wrote:
> ---
>  libavformat/mxfenc.c | 99 +-
> --
>  1 file changed, 55 insertions(+), 44 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 035e65ed43..ed6ecbf541 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -101,6 +101,12 @@ typedef struct MXFContainerEssenceEntry {
>  void (*write_desc)(AVFormatContext *, AVStream *);
>  } MXFContainerEssenceEntry;
>  
> +typedef struct MXFPackage {
> +char *name;
> +enum MXFMetadataSetType type;
> +int instance;
> +} MXFPackage;
> [...]

Looks OK. This would make it easier if someone is crazy enough to
implement the higher op types in the future. I still maintain that
libmxf/bmxlib is better for that however

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/4] avformat/mxfenc: use track count to generate component instance uuid

[Tomas Härdin]

On Sun, 2017-11-26 at 21:42 -0800, Mark Reid wrote:
> ---
>  libavformat/mxf.h   |  1 -
>  libavformat/mxfenc.c| 45 +
> 
>  tests/ref/fate/copy-trac4914|  2 +-
>  tests/ref/fate/time_base|  2 +-
>  tests/ref/lavf/mxf  |  6 +++---
>  tests/ref/lavf/mxf_d10  |  2 +-
>  tests/ref/lavf/mxf_dv25 |  2 +-
>  tests/ref/lavf/mxf_dvcpro50 |  2 +-
>  tests/ref/lavf/mxf_opatom   |  2 +-
>  tests/ref/lavf/mxf_opatom_audio |  2 +-
>  10 files changed, 38 insertions(+), 28 deletions(-)
> [...]
> @@ -846,6 +847,10 @@ static void
> mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage *packag
>  MXFContext *mxf = s->priv_data;
>  AVIOContext *pb = s->pb;
>  MXFStreamContext *sc = st->priv_data;
> +int instance = package->uuid_offset;
> +
> +if (st != mxf->timecode_track)
> +instance += st->index + 1;
>  
>  mxf_write_metadata_key(pb, 0x013b00);
>  PRINT_KEY(s, "track key", pb->buf_ptr - 16);
> 
>  static int mxf_write_essence_container_data(AVFormatContext *s)
> @@ -1443,11 +1451,12 @@ static int
> mxf_write_header_metadata_sets(AVFormatContext *s)
>  AVDictionaryEntry *entry = NULL;
>  AVStream *st = NULL;
>  int i;
> -
> +int track_count = 0;
>  MXFPackage packages[2] = {};
>  int package_count = 2;
>  packages[0].type = MaterialPackage;
>  packages[1].type = SourcePackage;
> +packages[1].instance = 1;
>  
>  if (entry = av_dict_get(s->metadata, "material_package_name",
> NULL, 0))
> packages[0].name = entry->value;
> @@ -1468,8 +1477,10 @@ static int
> mxf_write_header_metadata_sets(AVFormatContext *s)
>  mxf_write_preface(s);
>  mxf_write_identification(s);
>  mxf_write_content_storage(s, packages, package_count);
> -for (i = 0; i < package_count; i++)
> -mxf_write_package(s, [i]);
> +for (i = 0; i < package_count; i++) {
> +packages[i].uuid_offset = track_count;
> +track_count += mxf_write_package(s, [i]);
> +}

I see st->index + 1 when deriving instance from uuid_offset, are you
sure there isn't a potential off-by-one error here? An MP track and SP
track getting the same UUID or something. I guess type is enough to
differentiate, but then why does instance need uuid_offset?

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [RFC] [Vote] Drop Windows XP support

[Tomas Härdin]

On 2017-12-14 14:26, wm4 wrote:

I propose that FFmpeg sets the minimum supported Windows version to
Windows Vista (or maybe Windows 7). This would remove Windows XP
support.

The reason is that Windows XP does not provide certain convenient APIs,
in particular locking primitives that map well to pthread. There are
other problems, such as upstream projects dropping XP support, and
developers not being able to test.

Dropping it has advantages for us. For example, it would allow is to
rewrite the messy locking code for AVCodec registration, which has been
a topic recently.

Microsoft ended all support for Windows XP on January 31, 2009. That's
almost a decade ago. There is a lack of security updates, which makes
merely using Windows XP dangerous. There is no reason to put so much
effort into supporting an old, unsupported OS.

We should drop XP support, and allow unconditional use of Windows Vista
APIs.

I'm also casting an internal vote to probe for support (this vote is
meant for members of the FFmpeg voting committee only).

The subject of the vote is:

   Should we drop support for Windows XP starting in git master and the
   next FFmpeg major release?

Current release branches are explicitly unaffected.

My own vote on this issue is "yes" (assuming I still have a right for a
vote, I really don't know).


I'm not on the committee, but I'd vote "yes"

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3 1/3] avformat/mxfenc: use track count to generate component instance uuid

[Tomas Härdin]

On Thu, 2017-12-07 at 22:40 +0100, Michael Niedermayer wrote:
> On Thu, Dec 07, 2017 at 04:41:09PM +0100, Tomas Härdin wrote:
> > On 2017-12-05 05:46, Mark Reid wrote:
> > > @@ -1398,16 +1397,26 @@ static void
> > > mxf_write_package(AVFormatContext *s, MXFPackage *package)
> > >  mxf_write_uuid(pb, SubDescriptor, 0);
> > >  }
> > > +/*
> > > + * for every 1 track in a package there is 1 sequence and 1
> > > component.
> > > + * all 3 of these elements share the same instance number
> > > for generating
> > > + * there instance uuids. mxf->track_instance_count stores
> > > this value.
> > > + * mxf->track_instance_count is incremented after a group of
> > > all 3 of
> > > + * these elements are written.
> > > + */
> > > +
> > 
> > Excellent.
> > 
> > Patch looks good to me :)
> 
> any reason why you didnt apply it ?

No access. Plus waiting for feedback on my other comment

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3 2/3] avformat/mxfenc: write reel_name if metadata key is present

[Tomas Härdin]

On Thu, 2017-12-07 at 14:45 -0800, Mark Reid wrote:
> On Dec 7, 2017 7:45 AM, "Tomas Härdin" <tjop...@acc.umu.se> wrote:
> 
> On 2017-12-05 05:46, Mark Reid wrote:
> 
> > ---
> >   libavformat/mxf.h|  1 +
> >   libavformat/mxfenc.c | 42 +++--
> > -
> >   2 files changed, 36 insertions(+), 7 deletions(-)
> > 
> > 
> > @@ -1476,6 +1495,15 @@ static int
> > mxf_write_header_metadata_sets(AVFormatContext
> > *s)
> >   }
> >   }
> >   +entry = av_dict_get(s->metadata, "reel_name", NULL, 0);
> > +if (entry) {
> > +packages[2].name = entry->value;
> > +packages[2].type = SourcePackage;
> > +packages[2].instance = 2;
> > +packages[1].ref = [2];
> > +package_count = 3;
> > +}
> > 
> 
> I guess we can have it check track metadata later if we feel like it.
> Did a
> patch moving reel_name into AVFormatContext make it into mxfdec yet?
> mxfenc's output surviving a roundtrip through mxfdec + mxfenc might
> be
> desirable:
> 
> ffmpeg -i somefile_with_reel_name.mkv output.mxf
> ffmpeg -i output.mxf -vcodec copy -acodec copy remuxed.mxf
> 
> Ideally remuxed.mxf is identical to output.mxf
> 
> /Tomas
> 
> 
> Yes I agree that such behaviour is desirable.
> I haven't taken a look at mxfdec yet. Looking in steams for reel_name
> might
> be necessary, as I believe that's where mov,MP4 store it. But I was
> trying
> to keep it simple at first and addressed this specific issue a future
> patch.

Fair enough. I guess we can commit this patch series then. Michael?

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3 1/3] avformat/mxfenc: use track count to generate component instance uuid

[Tomas Härdin]

On 2017-12-05 05:46, Mark Reid wrote:

@@ -1398,16 +1397,26 @@ static void mxf_write_package(AVFormatContext *s, 
MXFPackage *package)
  mxf_write_uuid(pb, SubDescriptor, 0);
  }
  
+/*

+ * for every 1 track in a package there is 1 sequence and 1 component.
+ * all 3 of these elements share the same instance number for generating
+ * there instance uuids. mxf->track_instance_count stores this value.
+ * mxf->track_instance_count is incremented after a group of all 3 of
+ * these elements are written.
+ */
+


Excellent.

Patch looks good to me :)

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3 2/3] avformat/mxfenc: write reel_name if metadata key is present

[Tomas Härdin]

On 2017-12-05 05:46, Mark Reid wrote:

---
  libavformat/mxf.h|  1 +
  libavformat/mxfenc.c | 42 +++---
  2 files changed, 36 insertions(+), 7 deletions(-)


@@ -1476,6 +1495,15 @@ static int 
mxf_write_header_metadata_sets(AVFormatContext *s)
  }
  }
  
+entry = av_dict_get(s->metadata, "reel_name", NULL, 0);

+if (entry) {
+packages[2].name = entry->value;
+packages[2].type = SourcePackage;
+packages[2].instance = 2;
+packages[1].ref = [2];
+package_count = 3;
+}


I guess we can have it check track metadata later if we feel like it. 
Did a patch moving reel_name into AVFormatContext make it into mxfdec 
yet? mxfenc's output surviving a roundtrip through mxfdec + mxfenc might 
be desirable:


    ffmpeg -i somefile_with_reel_name.mkv output.mxf
    ffmpeg -i output.mxf -vcodec copy -acodec copy remuxed.mxf

Ideally remuxed.mxf is identical to output.mxf

/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3] avformat/mxfenc: add h264 profiles

[Tomas Härdin]

sön 2018-05-06 klockan 21:31 +0200 skrev Thomas Mundt:
> 2018-05-06 13:32 GMT+02:00 Tomas Härdin <tjop...@acc.umu.se>:
> 
> > fre 2018-05-04 klockan 01:52 +0200 skrev Thomas Mundt:
> > > Hi,
> > > 
> > > this is a better version of the patch.
> > > 10 bit and TFF are mandatory for AVC Intra only. Other profiles
> > > differ.
> > > 
> > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > index 3bb7032..81513dc 100644
> > > --- a/libavformat/mxfenc.c
> > > +++ b/libavformat/mxfenc.c
> > > @@ -1947,22 +1947,31 @@ static const struct {
> > >  int frame_size;
> > >  int profile;
> > >  uint8_t interlaced;
> > > +int long_gop;
> > 
> > A comment here explaining the difference between -1, 0 and 1 would
> > be
> > nice. The rest looks OK, but I didn't read the relevant specs to be
> > 100% sure
> > <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> > 
> 
> New patch attached.

Looks OK

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Add IRC nicknames to MAINTAINERS?

[Tomas Härdin]

ons 2018-04-25 klockan 11:42 +0200 skrev Paul B Mahol:
> On 4/25/18, Tomas Haerdin  wrote:
> > ons 2018-04-25 klockan 09:55 +0100 skrev Josh de Kock:
> > > On 2018/04/25 9:35, Paul B Mahol wrote:
> > > > On 4/25/18, Tomas Haerdin  wrote:
> > > > > [...]
> > > > > 
> > > > > I'll push this this some time later this week if I don't hear
> > > > > any
> > > > > objections
> > > > 
> > > > What is point of it if there is only one nickname?
> > > 
> > > There are two nicknames.
> > 
> > Suggest more then, or just push your own in the future
> 
> Looks like interest is very low...

Pushed what I had. Feel free to add your own nicknames if you like

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 03/13] avformat/mxfenc: Add Product Version, Toolkit version and Platform

[Tomas Härdin]

mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer 
>  static void mxf_write_identification(AVFormatContext *s)
>  {
>  MXFContext *mxf = s->priv_data;
> @@ -790,7 +809,7 @@ static void mxf_write_identification(AVFormatContext *s)
>  
>  version = s->flags & AVFMT_FLAG_BITEXACT ?
>  "0.0.0" : AV_STRINGIFY(LIBAVFORMAT_VERSION);
> -length = 72 + mxf_utf16_local_tag_length(company) +
> +length = 100 +mxf_utf16_local_tag_length(company) +

I have mixed feelings about this style of computation. But not enough
to hold the patch up, which looks OK

/Tomas



___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 01/13] avformat/mxfenc: Correct KAG alignment of preface

[Tomas Härdin]

mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> Signed-off-by: Michael Niedermayer 
> ---
>  libavformat/mxfenc.c|  1 +
>  tests/ref/fate/copy-trac4914|  4 ++--
>  tests/ref/fate/mxf-reel_name|  2 +-
>  tests/ref/fate/time_base|  2 +-
>  tests/ref/lavf/mxf  | 12 ++--
>  tests/ref/lavf/mxf_d10  |  4 ++--
>  tests/ref/lavf/mxf_dv25 |  4 ++--
>  tests/ref/lavf/mxf_dvcpro50 |  4 ++--
>  tests/ref/lavf/mxf_opatom   |  2 +-
>  tests/ref/lavf/mxf_opatom_audio |  4 ++--
>  10 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 3bb70326fe..c0db10b3c2 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -1757,6 +1757,7 @@ static int mxf_write_partition(AVFormatContext
> *s, int bodysid,
>  mxf_write_klv_fill(s);
>  start = avio_tell(s->pb);
>  mxf_write_primer_pack(s);
> +mxf_write_klv_fill(s);
>  mxf_write_header_metadata_sets(s);
>  pos = avio_tell(s->pb);
>  header_byte_count = pos - start + klv_fill_size(pos);

Feels like such an elementary error. Probably OK, but it's been a while
since I read the specs

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] github

[Tomas Härdin]

fre 2018-04-27 klockan 00:50 +0200 skrev wm4:
> On Thu, 26 Apr 2018 14:41:55 +0200
> Daniel Oberhoff  wrote:
> 
> > > On 26. Apr 2018, at 14:40, wm4  wrote:
> > > 
> > > On Thu, 26 Apr 2018 14:12:14 +0200
> > > Hendrik Leppkes 
> > > > wrote:
> > >   
> > > > On Thu, Apr 26, 2018 at 2:02 PM, Daniel Oberhoff
> > > >  wrote:  
> > > > >   
> > > > > > Am 26.04.2018 um 13:59 schrieb Daniel Oberhoff  > > > > > o...@googlemail.com>:
> > > > > > 
> > > > > >   
> > > > > > > Am 26.04.2018 um 13:56 schrieb Daniel Oberhoff  > > > > > > rh...@googlemail.com>:
> > > > > > > 
> > > > > > >   
> > > > > > > > Am 26.04.2018 um 13:52 schrieb Nicolas George  > > > > > > > sup.org>:
> > > > > > > > 
> > > > > > > > Daniel Oberhoff (2018-04-26):  
> > > > > > > > > I was wondering if there is any chance to move
> > > > > > > > > development to github?
> > > > > > > > > I.e. not just mirror, but as primary development
> > > > > > > > > repo, with issues and
> > > > > > > > > pull requests? Would make collaboration a *lot*
> > > > > > > > > easier (think of
> > > > > > > > > submitting a pr instead of having to
> > > > > > > > > generate/format/split patches).  
> > > > > > > > 
> > > > > > > > If development involves working in a web browser a lot,
> > > > > > > > count me out.
> > > > > > > > Can you point me to the command-line  
> > > > > > > 
> > > > > > > https://hub.github.com/hub.1.html  
> > > > > > 
> > > > > > But you can’t really do reviews that way, so the criticism
> > > > > > stands.  
> > > > > 
> > > > > BTW, is there any kind of issue tracking?  
> > > > 
> > > > https://trac.ffmpeg.org/  
> > > 
> > > To be fair, I'd prefer the github issue tracker over TRAC any
> > > day.
> > > Still has the other problems I mentioned.  
> > 
> > gitlab?
> > 
> 
> That would mostly get rid of the centralization argument. But I've
> heard bad things from someone who wanted to setup a private instance
> of
> it. Apparently it has a large number of dependencies, is extremely
> hard
> to deploy (unless you use their docker container), and it's SLOW.
> 
> In fact even gitlab.com seems to have severe performance problems
> occasionally.

Another problem with gitlab is its inability to work without js. Things
like hamburger menus getting in the way, READMEs not displaying and so
on. Just look at https://gitlab.com/explore with js and third party
domains disabled

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3] avformat/mxfenc: add h264 profiles

[Tomas Härdin]

tis 2018-05-08 klockan 18:32 +0200 skrev Thomas Mundt:
> 2018-05-07 10:40 GMT+02:00 Tomas Härdin <tjop...@acc.umu.se>:
> 
> > sön 2018-05-06 klockan 21:31 +0200 skrev Thomas Mundt:
> > > 2018-05-06 13:32 GMT+02:00 Tomas Härdin <tjop...@acc.umu.se>:
> > > 
> > > > fre 2018-05-04 klockan 01:52 +0200 skrev Thomas Mundt:
> > > > > Hi,
> > > > > 
> > > > > this is a better version of the patch.
> > > > > 10 bit and TFF are mandatory for AVC Intra only. Other
> > > > > profiles
> > > > > differ.
> > > > > 
> > > > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > > > index 3bb7032..81513dc 100644
> > > > > --- a/libavformat/mxfenc.c
> > > > > +++ b/libavformat/mxfenc.c
> > > > > @@ -1947,22 +1947,31 @@ static const struct {
> > > > >  int frame_size;
> > > > >  int profile;
> > > > >  uint8_t interlaced;
> > > > > +int long_gop;
> > > > 
> > > > A comment here explaining the difference between -1, 0 and 1
> > > > would
> > > > be
> > > > nice. The rest looks OK, but I didn't read the relevant specs
> > > > to be
> > > > 100% sure
> > > > <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> > > > 
> > > 
> > > New patch attached.
> > 
> > Looks OK
> 
> 
> Thanks,
> 
> can you or someone else push it please.

Could you attach a patch that applies cleanly? I tried to git am it on
commit 0612e29 but not luck

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 06/13] avformat/mxfenc: Add Sample width/height/x offset/y offset, Display x offset and F2 offset

[Tomas Härdin]

tis 2018-05-08 klockan 18:26 +0200 skrev Michael Niedermayer:
> On Tue, May 08, 2018 at 12:40:49PM +0200, Tomas Härdin wrote:
> > mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> > > > Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc>
> > > 
> > > +if (sc->interlaced) {
> > > +//Display F2 Offset
> > > +mxf_write_local_tag(pb, 4, 0x3217);
> > > +avio_wb32(pb, -((st->codecpar->height -
> > > display_height)&1));
> > 
> > Negative values for DisplayF2Offset are not valid
> 
> The specification (SMPTE 377-1-2009) says:
> 
> The DisplayF2Offset Property adjusts the DisplayYOffset for the
> second field relative to that for the first field. Its
> value shall be zero (0) or minus 1. A value of minus 1 shall invert
> the Displayed Topness relative to the Sampled
> Topness.

Huh, the document I have is from 2004, which says "If the property is
not present, its value shall be assumed to be 0. Valid values are zero
or 1.". So I guess they changed that. I guess it's fine then.

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3] avformat/mxfenc: add h264 profiles

[Tomas Härdin]

tor 2018-05-10 klockan 22:44 +0200 skrev Tomas Härdin:
> tis 2018-05-08 klockan 18:32 +0200 skrev Thomas Mundt:
> > 2018-05-07 10:40 GMT+02:00 Tomas Härdin <tjop...@acc.umu.se>:
> > 
> > > sön 2018-05-06 klockan 21:31 +0200 skrev Thomas Mundt:
> > > > 2018-05-06 13:32 GMT+02:00 Tomas Härdin <tjop...@acc.umu.se>:
> > > > 
> > > > > fre 2018-05-04 klockan 01:52 +0200 skrev Thomas Mundt:
> > > > > > Hi,
> > > > > > 
> > > > > > this is a better version of the patch.
> > > > > > 10 bit and TFF are mandatory for AVC Intra only. Other
> > > > > > profiles
> > > > > > differ.
> > > > > > 
> > > > > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > > > > index 3bb7032..81513dc 100644
> > > > > > --- a/libavformat/mxfenc.c
> > > > > > +++ b/libavformat/mxfenc.c
> > > > > > @@ -1947,22 +1947,31 @@ static const struct {
> > > > > >  int frame_size;
> > > > > >  int profile;
> > > > > >  uint8_t interlaced;
> > > > > > +int long_gop;
> > > > > 
> > > > > A comment here explaining the difference between -1, 0 and 1
> > > > > would
> > > > > be
> > > > > nice. The rest looks OK, but I didn't read the relevant specs
> > > > > to be
> > > > > 100% sure
> > > > > <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> > > > > 
> > > > 
> > > > New patch attached.
> > > 
> > > Looks OK
> > 
> > 
> > Thanks,
> > 
> > can you or someone else push it please.
> 
> Could you attach a patch that applies cleanly? I tried to git am it
> on
> commit 0612e29 but not luck

Never mind, I just saw Michael applied this

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 05/13] avformat/mxfenc: Fix stored width

[Tomas Härdin]

mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> This fixes the width to have computations matching the height
> 
> > Signed-off-by: Michael Niedermayer 
> ---
>  libavformat/mxfenc.c  |   3 +-
>  .../ref/fate/concat-demuxer-extended-lavf-mxf |   2 +-
>  .../fate/concat-demuxer-extended-lavf-mxf_d10 |   2 +-
>  .../ref/fate/concat-demuxer-simple1-lavf-mxf  | 242 +-
>  .../fate/concat-demuxer-simple1-lavf-mxf_d10  | 140 +-
>  tests/ref/seek/lavf-mxf   |  44 ++--
>  tests/ref/seek/lavf-mxf_d10   |  54 ++--
>  tests/ref/seek/lavf-mxf_dv25  |  54 ++--
>  tests/ref/seek/lavf-mxf_dvcpro50  |  54 ++--
>  tests/ref/seek/lavf-mxf_opatom_audio  |  54 ++--
>  10 files changed, 325 insertions(+), 324 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index f0fd406493..9140302b81 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -1131,6 +1131,7 @@ static void mxf_write_cdci_common(AVFormatContext *s, 
> AVStream *st, const UID ke
>  {
>  MXFStreamContext *sc = st->priv_data;
>  AVIOContext *pb = s->pb;
> +int stored_width  = (st->codecpar->width +15)/16*16;
>  int stored_height = (st->codecpar->height+15)/16*16;

Should a muxer really do this kinds of computations? What happens if a
codec comes along that has larger or smaller macroblocks?

>  int display_height;
>  int f1, f2;
> @@ -1143,7 +1144,7 @@ static void mxf_write_cdci_common(AVFormatContext *s, 
> AVStream *st, const UID ke
>  mxf_write_generic_desc(s, st, key, desc_size);
>  
>  mxf_write_local_tag(pb, 4, 0x3203);
> -avio_wb32(pb, st->codecpar->width);
> +avio_wb32(pb, stored_width);

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 11/13] avformat/mxfenc: Write Audio Ref Level for D10

[Tomas Härdin]

mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer 
>  // Generic Sound Essence Descriptor
>  { 0x3D02, 
> {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x04,0x04,0x02,0x03,0x01,0x04,0x00,0x00,0x00}},
>  /* Locked/Unlocked */
>  { 0x3D03, 
> {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x05,0x04,0x02,0x03,0x01,0x01,0x01,0x00,0x00}},
>  /* Audio sampling rate */
> +{ 0x3D04, 
> {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x01,0x04,0x02,0x01,0x01,0x03,0x00,0x00,0x00}},
>  /* Audio Ref Level */
>  { 0x3D07, 
> {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x05,0x04,0x02,0x01,0x01,0x04,0x00,0x00,0x00}},
>  /* ChannelCount */
>  { 0x3D01, 
> {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x04,0x04,0x02,0x03,0x03,0x04,0x00,0x00,0x00}},
>  /* Quantization bits */
>  { 0x3D06, 
> {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x04,0x02,0x04,0x02,0x00,0x00,0x00,0x00}},
>  /* Sound Essence Compression */
> @@ -1333,6 +1334,8 @@ static void 
> mxf_write_generic_sound_common(AVFormatContext *s, AVStream *st, con
>  
>  if (s->oformat == _mxf_opatom_muxer)
>  duration_size = 12;
> +if (s->oformat == _mxf_d10_muxer)
> +size += 5;
>  
>  mxf_write_generic_desc(s, st, key, size+duration_size+5+12+8+8);
>  
> @@ -1350,6 +1353,11 @@ static void 
> mxf_write_generic_sound_common(AVFormatContext *s, AVStream *st, con
>  avio_wb32(pb, st->codecpar->sample_rate);
>  avio_wb32(pb, 1);
>  
> +if (s->oformat == _mxf_d10_muxer) {
> +mxf_write_local_tag(pb, 1, 0x3D04);
> +avio_w8(pb, 0);
> +}

Sizes and such look OK, but again I don't know what the D-10 spec says

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 07/13] avformat/mxfenc: Add vertical subsampling support

[Tomas Härdin]

mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer 
> 
> @@ -1149,6 +1151,8 @@ static void mxf_write_cdci_common(AVFormatContext *s, 
> AVStream *st, const UID ke
>  desc_size += 5;
>  if (sc->interlaced)
>  desc_size += 8;
> +if (sc->v_chroma_sub_sample)
> +desc_size += 8;
>  
>  mxf_write_generic_desc(s, st, key, desc_size);
>  
> @@ -1209,6 +1213,12 @@ static void mxf_write_cdci_common(AVFormatContext *s, 
> AVStream *st, const UID ke
>  mxf_write_local_tag(pb, 4, 0x3302);
>  avio_wb32(pb, sc->h_chroma_sub_sample);
>  
> +// vertical subsampling
> +if (sc->v_chroma_sub_sample) {
> +mxf_write_local_tag(pb, 4, 0x3308);
> +avio_wb32(pb, sc->v_chroma_sub_sample);
> +}
> +
>  // color siting
>  mxf_write_local_tag(pb, 1, 0x3303);
>  avio_w8(pb, sc->color_siting);
> @@ -2273,6 +2283,7 @@ static int mxf_write_header(AVFormatContext *s)
>  if (pix_desc) {
>  sc->component_depth = pix_desc->comp[0].depth;
>  sc->h_chroma_sub_sample = 1 << pix_desc->log2_chroma_w;
> +sc->v_chroma_sub_sample = 1 << pix_desc->log2_chroma_h;

Looks OK. Had this confused for chrome siting for a while, but I see
that is actually handled correctly.

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 08/13] avformat/mxfenc: add white/black ref /color range

[Tomas Härdin]

mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer 
> 
> +if (st->codecpar->color_range != AVCOL_RANGE_UNSPECIFIED) {
> +int black = 0,
> +white = (1 +color = (1 +if (st->codecpar->color_range == AVCOL_RANGE_MPEG) {
> +black = 1   << (sc->component_depth - 4);
> +white = 235 << (sc->component_depth - 8);
> +color = (14 << (sc->component_depth - 4)) + 1;
> +}

This feels like something that should be part of pix_fmt stuff or
something. But maybe someone can refactor that later. Looks OK
otherwise

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/13] avformat/mxfenc: Set color siting to 0 for D10-MXF

[Tomas Härdin]

mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer 
> ---
>  libavformat/mxfenc.c  |   1 +
>  .../ref/fate/concat-demuxer-extended-lavf-mxf |   2 +-
>  .../fate/concat-demuxer-extended-lavf-mxf_d10 |   2 +-
>  .../ref/fate/concat-demuxer-simple1-lavf-mxf  | 242 +-
>  .../fate/concat-demuxer-simple1-lavf-mxf_d10  | 140 +-
>  tests/ref/seek/lavf-mxf   |  44 ++--
>  tests/ref/seek/lavf-mxf_d10   |  54 ++--
>  tests/ref/seek/lavf-mxf_dv25  |  54 ++--
>  tests/ref/seek/lavf-mxf_dvcpro50  |  54 ++--
>  tests/ref/seek/lavf-mxf_opatom|  54 ++--
>  tests/ref/seek/lavf-mxf_opatom_audio  |  54 ++--
>  11 files changed, 351 insertions(+), 350 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index f2be76cc86..adf5527534 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2361,6 +2361,7 @@ static int mxf_write_header(AVFormatContext *s)
>  mxf->edit_unit_byte_count += 
> klv_fill_size(mxf->edit_unit_byte_count);
>  
>  sc->signal_standard = 1;
> +sc->color_siting = 0;
>  }

Can't find anything in my documents that says anything about this. I
don't remember what D-10 is actually specified in...

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 13/13] avformat/mxfenc: Write transfer characteristic

[Tomas Härdin]

mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer 
> 
> +static int get_trc(UID ul, enum AVColorTransferCharacteristic trc)
> +{
> +switch (trc){
> +case AVCOL_TRC_GAMMA28   :
> +case AVCOL_TRC_GAMMA22   :
> +memcpy(ul, 
> (UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x01,0x01,0x01,0x01,0x00,0x00},
>  16);
> +return 0;
> +case AVCOL_TRC_BT709 :
> +case AVCOL_TRC_SMPTE170M :
> +memcpy(ul, 
> (UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x01,0x01,0x01,0x02,0x00,0x00},
>  16);
> +return 0;
> +case AVCOL_TRC_SMPTE240M :
> +memcpy(ul, 
> (UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x01,0x01,0x01,0x03,0x00,0x00},
>  16);
> +return 0;
> +case AVCOL_TRC_BT1361_ECG:
> +memcpy(ul, 
> (UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x06,0x04,0x01,0x01,0x01,0x01,0x05,0x00,0x00},
>  16);
> +return 0;
> +case AVCOL_TRC_LINEAR:
> +memcpy(ul, 
> (UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x06,0x04,0x01,0x01,0x01,0x01,0x06,0x00,0x00},
>  16);
> +return 0;
> +case AVCOL_TRC_SMPTE428  :
> +memcpy(ul, 
> (UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x08,0x04,0x01,0x01,0x01,0x01,0x07,0x00,0x00},
>  16);
> +return 0;
> +default:
> +return -1;
> +}
> +}
> +
>  static void mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const 
> UID key, unsigned size)
>  {
>  MXFStreamContext *sc = st->priv_data;
> @@ -1152,6 +1181,8 @@ static void mxf_write_cdci_common(AVFormatContext *s, 
> AVStream *st, const UID ke
>  int display_height;
>  int f1, f2;
>  unsigned desc_size = size+8+8+8+8+8+8+8+5+16+4+12+20+5 + 5*8 + 6;
> +UID transfer_ul = {0};
> +
>  if (sc->interlaced && sc->field_dominance)
>  desc_size += 5;
>  if (sc->signal_standard)
> @@ -1164,6 +1195,8 @@ static void mxf_write_cdci_common(AVFormatContext *s, 
> AVStream *st, const UID ke
>  desc_size += 8 * 3;
>  if (s->oformat == _mxf_d10_muxer)
>  desc_size += 8 + 8 + 8;
> +if (get_trc(transfer_ul, st->codecpar->color_trc) >= 0)
> +desc_size += 20;
>  
>  mxf_write_generic_desc(s, st, key, desc_size);
>  
> @@ -1305,6 +1338,12 @@ static void mxf_write_cdci_common(AVFormatContext *s, 
> AVStream *st, const UID ke
>  avio_wb32(pb, sc->aspect_ratio.num);
>  avio_wb32(pb, sc->aspect_ratio.den);
>  
> +//Transfer characteristic
> +if (transfer_ul[0]) {
> +mxf_write_local_tag(pb, 16, 0x3210);
> +avio_write(pb, transfer_ul, 16);
> +};

Looks OK, but I didn't check the ULs for correctness

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 12/13] avformat/mxfenc: Add Stored F2 Offset / Image Start/End Offset for D10

[Tomas Härdin]

mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer 
>  desc_size += 8;
>  if (st->codecpar->color_range != AVCOL_RANGE_UNSPECIFIED)
>  desc_size += 8 * 3;
> +if (s->oformat == _mxf_d10_muxer)
> +desc_size += 8 + 8 + 8;
>  
>  mxf_write_generic_desc(s, st, key, desc_size);
>  
> @@ -1169,6 +1173,20 @@ static void mxf_write_cdci_common(AVFormatContext *s, 
> AVStream *st, const UID ke
>  mxf_write_local_tag(pb, 4, 0x3202);
>  avio_wb32(pb, stored_height>>sc->interlaced);
>  
> +if (s->oformat == _mxf_d10_muxer) {
> +//Stored F2 Offset
> +mxf_write_local_tag(pb, 4, 0x3216);
> +avio_wb32(pb, 0);
> +
> +//Image Start Offset
> +mxf_write_local_tag(pb, 4, 0x3213);
> +avio_wb32(pb, 0);
> +
> +//Image End Offset
> +mxf_write_local_tag(pb, 4, 0x3214);
> +avio_wb32(pb, 0);
> +}

Looks OK, but of course I don't know what the spec says

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 04/13] avformat/mxfenc: Add object model version

[Tomas Härdin]

mån 2018-05-07 klockan 12:38 +0200 skrev Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer 

>  mxf_write_metadata_key(pb, 0x012f00);
>  PRINT_KEY(s, "preface key", pb->buf_ptr - 16);
> -klv_encode_ber_length(pb, 130 + 16LL * 
> DESCRIPTOR_COUNT(mxf->essence_container_count));
> +klv_encode_ber_length(pb, 138 + 16LL * 
> DESCRIPTOR_COUNT(mxf->essence_container_count));
>  
>  // write preface set uid
>  mxf_write_local_tag(pb, 16, 0x3C0A);
> @@ -696,6 +697,10 @@ static void mxf_write_preface(AVFormatContext *s)
>  mxf_write_local_tag(pb, 2, 0x3B05);
>  avio_wb16(pb, 259); // v1.2
>  
> +// Object Model Version
> +mxf_write_local_tag(pb, 4, 0x3B07);
> +avio_wb32(pb, 1);
> 

Not sure what use this is, but looks OK at least. S377m doesn't seem to
think it's necessary. Is there some program that needs this?

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel