[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 <tjoppen at 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