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

[Marton Balint]

On Thu, 22 Feb 2018, Paul B Mahol wrote:

> 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!

I plan to apply the series soon, unless there are other comments :)

Thanks,
Marton