On Fri, Nov 28, 2014 at 9:26 AM, Tomas Härdin <tomas.har...@codemill.se>
wrote:
> 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?
>
I was looking for a AV_PIX_FMT_A8 or something like that but could find
one.
http://ffmpeg.org/doxygen/trunk/pixfmt_8h.html#a9a8e335cf3be472042bc9f0cf80cd4c5
AV_PIX_FMT_GRAY8 has the right buffer size, 1 channel 8bpp.
AV_PIX_FMT_YA8 or AV_PIX_FMT_GRAY8A are 4 channels.
> > };
> >
> > 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.
>
Okay I'll do that, I just didn't because of the scary comment at the bottom
of the enum
TypeBottom,// add metadata type before this
> > 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.
>
I copied the same code from the mxf_read_sequence func, so I assumed it was
okay.
http://ffmpeg.org/doxygen/trunk/mem_8c_source.html#l00251
av_calloc does check for a overflow and will return NULL if nmemb >=
INT_MAX / size. So it should fail before even getting to the avio_read.
> > +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.
>
>
I was planing of removing this in a future patch. I was going to move the
logic into mxf_parse_structural_metadata, but need to refactor and shuffle
somethings around in there to get it to work.
> > +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.
>
Great, I'll double check it with valgrind, remove the changes to the tests,
put EssenceGroup at the end of the enum and send a new patch.
thanks for taking the time to review.
> /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
