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

2017-11-28 Thread Tomas Härdin

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

On Mon, Nov 27, 2017 at 2:40 AM, Tomas Härdin  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 3/4] avformat/mxfenc: write reel_name if metadata key is present

2017-11-27 Thread Mark Reid
On Mon, Nov 27, 2017 at 2:40 AM, Tomas Härdin  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?



> /Tomas
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
___
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

2017-11-27 Thread 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