[FFmpeg-devel] [PATCH 02/10] avformat/mxfdec: fix essence_offset calculation

[Paul B Mahol]

On 2/22/18, Tomas Haerdin <tjoppen at acc.umu.se> wrote:
> ons 2018-02-21 klockan 23:06 +0100 skrev Marton Balint:
>> On Wed, 21 Feb 2018, Tomas Haerdin wrote:
>>
>> > loer 2018-02-17 klockan 22:45 +0100 skrev Marton Balint:
>> > > The reference point for a KAG is the first byte of the key of a
>> > > Partition Pack.
>> > >
>> > > Fixes ticket #2817.
>> > > Fixes ticket #5317.
>> > >
>> > > > Signed-off-by: Marton Balint <cus at passwd.hu>
>> > >
>> > > ---
>> > >  libavformat/mxfdec.c | 4 ++--
>> > >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> > > index fcae863ef4..95767ccba4 100644
>> > > --- a/libavformat/mxfdec.c
>> > > +++ b/libavformat/mxfdec.c
>> > > @@ -2875,8 +2875,8 @@ static int mxf_read_header(AVFormatContext *s)
>> > >                   *       for OPAtom we still need the actual
>> > > essence_offset though (the KL's length can vary)
>> > >                   */
>> > >                  int64_t op1a_essence_offset =
>> > > -                    round_to_kag(mxf->current_partition->this_partition
>> > > +
>> > > -                                 mxf->current_partition->pack_length,       mxf->current_partition->kag_size)
>> > > +
>> > > +                    mxf->current_partition->this_partition +
>> > > +                    round_to_kag(mxf->current_partition->pack_length,       mxf->current_partition->kag_size)
>> > > +
>> > >                      round_to_kag(mxf->current_partition->header_byte_count,
>> > > mxf->current_partition->kag_size) +
>> > >                      round_to_kag(mxf->current_partition->index_byte_count,  mxf->current_partition->kag_size);
>> >
>> > This seems like a rather elemental miscalculation, that I wrote even. I
>> > took a look at S377m, it had this to say:
>> >
>> > > The first gridline in any partition is the first byte of the key of
>> > > the partition pack that defines that
>> > > partition.
>> >
>> > Each partition starts at ThisPartition (plus run-in) so this patch
>> > should be correct. What's perhaps more suspect is the calculation
>> > itself. It should *always* be possible to locate where Op1a essence
>> > starts, by scanning to the first essence KLV. MXF is flexible enough
>> > that having some sketchy calculation for where it *might* be is
>> > probably not correct. One is free to insert any amount of padding
>>
>> The next patch more or less handles this by checking for a system item
>> key and explicitly setting the offset if that is found. An essence alone
>> however might not be the first essence, it is also possible that we
>> already skipped an unknown essence key, or an unknown system item key, so
>> I decided to keep the code as is if the first recognized essence is not a
>> system item.
>
> Sounds reasonable I guess. I'm going to reiterate that I consider
> continuing to maintain mxfdec is a mistake. A wrapper around
> bmxlib/libMXF would be much easier to maintain

YOu are lazy and evil! People please ignore him!