| Message ID | alpine.LSU.2.20.1806132049270.17215@iq |
|---|---|
| State | New |
| Headers | show |
ons 2018-06-13 klockan 21:58 +0200 skrev Marton Balint: > > On Wed, 13 Jun 2018, Tomas Härdin wrote: > > > sön 2018-06-10 klockan 12:36 +0200 skrev Marton Balint: > > > Also compute the correct essence_offset and essence_length for all clip wrapped > > > essences. > > > > > > > Signed-off-by: Marton Balint <cus@passwd.hu> > > > > > > --- > > > libavformat/mxfdec.c | 108 +++++++++++++++++++++++++++------------------------ > > > 1 file changed, 57 insertions(+), 51 deletions(-) > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > index 1ab34f4873..67b0028e88 100644 > > > --- a/libavformat/mxfdec.c > > > +++ b/libavformat/mxfdec.c > > > @@ -98,6 +98,7 @@ typedef struct MXFPartition { > > > int pack_length; > > > int64_t pack_ofs; ///< absolute offset of pack in file, including run-in > > > int64_t body_offset; > > > + KLVPacket first_essence_klv; > > > } MXFPartition; > > > > > > typedef struct MXFCryptoContext { > > > @@ -2782,47 +2783,76 @@ static int mxf_parse_handle_partition_or_eof(MXFContext *mxf) > > > return mxf->parsing_backward ? mxf_seek_to_previous_partition(mxf) : 1; > > > } > > > > > > +static int64_t round_to_kag(int64_t position, int kag_size) > > > +{ > > > + /* TODO: account for run-in? the spec isn't clear whether KAG should account for it */ > > > + /* NOTE: kag_size may be any integer between 1 - 2^10 */ > > > + int64_t ret = (position / kag_size) * kag_size; > > > + return ret == position ? ret : ret + kag_size; > > > > This should probably just be > > > > return ((position + kag_size - 1) / kag_size) * kag_size; > > > > but maybe there's some fine point I'm overlooking. What happens if > > kag_size is a smallish value like 16? > > > > A check for position > INT64_MAX - kag_size might also be appropriate. > > Well, this is your code as well :), I just moved it to avoid a forward > declaration. > > > > > > +} > > > + > > > +static MXFWrappingScheme mxf_get_wrapping_by_body_sid(AVFormatContext *s, int body_sid) > > > +{ > > > + for (int i = 0; i < s->nb_streams; i++) { > > > + MXFTrack *track = s->streams[i]->priv_data; > > > + if (track && track->body_sid == body_sid && track->wrapping != UnknownWrapped) > > > + return track->wrapping; > > > + } > > > + return UnknownWrapped; > > > +} > > > + > > > /** > > > * Figures out the proper offset and length of the essence container in each partition > > > */ > > > -static void mxf_compute_essence_containers(MXFContext *mxf) > > > +static void mxf_compute_essence_containers(AVFormatContext *s) > > > { > > > + MXFContext *mxf = s->priv_data; > > > int x; > > > > > > - /* everything is already correct */ > > > - if (mxf->op == OPAtom) > > > - return; > > > - > > > for (x = 0; x < mxf->partitions_count; x++) { > > > MXFPartition *p = &mxf->partitions[x]; > > > + MXFWrappingScheme wrapping; > > > > > > if (!p->body_sid) > > > continue; /* BodySID == 0 -> no essence */ > > > > > > - if (x >= mxf->partitions_count - 1) > > > - break; /* FooterPartition - can't compute length (and we don't need to) */ > > > + /* for non clip-wrapped essences we compute essence_offset > > > + * for clip wrapped essences we point essence_offset after the KL (usually klv.offset + 20 or 25) > > > + */ > > > > > > - /* essence container spans to the next partition */ > > > - p->essence_length = mxf->partitions[x+1].this_partition - p->essence_offset; > > > + wrapping = (mxf->op == OPAtom) ? ClipWrapped : mxf_get_wrapping_by_body_sid(s, p->body_sid); > > > > > > - if (p->essence_length < 0) { > > > - /* next ThisPartition < essence_offset */ > > > - p->essence_length = 0; > > > - av_log(mxf->fc, AV_LOG_ERROR, > > > - "partition %i: bad ThisPartition = %"PRIX64"\n", > > > - x+1, mxf->partitions[x+1].this_partition); > > > + if (wrapping == ClipWrapped) { > > > + p->essence_offset = p->first_essence_klv.next_klv - p->first_essence_klv.length; > > > + p->essence_length = p->first_essence_klv.length; > > > > Much nicer > > > > > + } else { > > > + int64_t op1a_essence_offset = > > > + p->this_partition + > > > + round_to_kag(p->pack_length, p->kag_size) + > > > + round_to_kag(p->header_byte_count, p->kag_size) + > > > + round_to_kag(p->index_byte_count, p->kag_size); > > > > Is this really always the case? I guess with OP1a it isn't a huge > > concern since the demuxer will find the next essence packet anyway. But > > still.. > > > > I'm also fairly sure this is my code originally so.. :) > > I think this tends to work well, if kag_size is not guessed. However, if > it _is_ guessed, then there might be problems, and having a 0 kagsize is > valid (only means an unknown KAG). So I am inclined to simply > use p->first_essence_klv.offset unconditionally, as you suggested in > > http://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/225629.html Yes, past me seems to have the right idea :) > See the attached patch. I think we should still parse kag_size even if we don't end up using it for much. Especially since there's special cases for Sony files in there. Did we have a sample for that? I forget. Are there cases where we might not run across an essence KLV during scanning but where there would be one anyway? I suspect not, not even if we're reading an unseekable file /Tomas
On Thu, 14 Jun 2018, Tomas Härdin wrote: [...] >> > > + } else { >> > > + int64_t op1a_essence_offset = >> > > + p->this_partition + >> > > + round_to_kag(p->pack_length, p->kag_size) + >> > > + round_to_kag(p->header_byte_count, p->kag_size) + >> > > + round_to_kag(p->index_byte_count, p->kag_size); >> > >> > Is this really always the case? I guess with OP1a it isn't a huge >> > concern since the demuxer will find the next essence packet anyway. But >> > still.. >> > >> > I'm also fairly sure this is my code originally so.. :) >> >> I think this tends to work well, if kag_size is not guessed. However, if >> it _is_ guessed, then there might be problems, and having a 0 kagsize is >> valid (only means an unknown KAG). So I am inclined to simply >> use p->first_essence_klv.offset unconditionally, as you suggested in >> >> http://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/225629.html > > Yes, past me seems to have the right idea :) > >> See the attached patch. > > I think we should still parse kag_size even if we don't end up using it > for much. Especially since there's special cases for Sony files in > there. Did we have a sample for that? I forget. fate-suite/mxf/C0023S01.mxf > > Are there cases where we might not run across an essence KLV during > scanning but where there would be one anyway? I suspect not, not even > if we're reading an unseekable file No, I don't think so either. Regards, Marton
From 6ff3c549fa78caeb8bfddfbee9961ecd59ca7c5f Mon Sep 17 00:00:00 2001 From: Marton Balint <cus@passwd.hu> Date: Wed, 13 Jun 2018 21:46:34 +0200 Subject: [PATCH] avformat/mxfdec: simply use the first essence element for non frame-wrapped partition essence offset Also add the canopus essence element to the list of the recognized essence element keys. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavformat/mxfdec.c | 33 ++------------------------------- 1 file changed, 2 insertions(+), 31 deletions(-) diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index 8f43ef46b9..2ec778e280 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -719,18 +719,6 @@ static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size mxf->op = OP1a; } - if (partition->kag_size <= 0 || partition->kag_size > (1 << 20)) { - av_log(mxf->fc, AV_LOG_WARNING, "invalid KAGSize %"PRId32" - guessing ", - partition->kag_size); - - if (mxf->op == OPSONYOpt) - partition->kag_size = 512; - else - partition->kag_size = 1; - - av_log(mxf->fc, AV_LOG_WARNING, "%"PRId32"\n", partition->kag_size); - } - return 0; } @@ -2800,14 +2788,6 @@ static int mxf_parse_handle_partition_or_eof(MXFContext *mxf) return mxf->parsing_backward ? mxf_seek_to_previous_partition(mxf) : 1; } -static int64_t round_to_kag(int64_t position, int kag_size) -{ - /* TODO: account for run-in? the spec isn't clear whether KAG should account for it */ - /* NOTE: kag_size may be any integer between 1 - 2^10 */ - int64_t ret = (position / kag_size) * kag_size; - return ret == position ? ret : ret + kag_size; -} - static MXFWrappingScheme mxf_get_wrapping_by_body_sid(AVFormatContext *s, int body_sid) { for (int i = 0; i < s->nb_streams; i++) { @@ -2843,17 +2823,7 @@ static void mxf_compute_essence_containers(AVFormatContext *s) p->essence_offset = p->first_essence_klv.next_klv - p->first_essence_klv.length; p->essence_length = p->first_essence_klv.length; } else { - int64_t op1a_essence_offset = - p->this_partition + - round_to_kag(p->pack_length, p->kag_size) + - round_to_kag(p->header_byte_count, p->kag_size) + - round_to_kag(p->index_byte_count, p->kag_size); - - /* NOTE: op1a_essence_offset may be less than to klv.offset (C0023S01.mxf) */ - if (IS_KLV_KEY(p->first_essence_klv.key, mxf_system_item_key_cp) || IS_KLV_KEY(p->first_essence_klv.key, mxf_system_item_key_gc)) - p->essence_offset = p->first_essence_klv.offset; - else - p->essence_offset = op1a_essence_offset; + p->essence_offset = p->first_essence_klv.offset; /* essence container spans to the next partition */ if (x < mxf->partitions_count - 1) @@ -3066,6 +3036,7 @@ static int mxf_read_header(AVFormatContext *s) av_log(s, AV_LOG_TRACE, "size %"PRIu64" offset %#"PRIx64"\n", klv.length, klv.offset); if (IS_KLV_KEY(klv.key, mxf_encrypted_triplet_key) || IS_KLV_KEY(klv.key, mxf_essence_element_key) || + IS_KLV_KEY(klv.key, mxf_canopus_essence_element_key) || IS_KLV_KEY(klv.key, mxf_avid_essence_element_key) || IS_KLV_KEY(klv.key, mxf_system_item_key_cp) || IS_KLV_KEY(klv.key, mxf_system_item_key_gc)) { -- 2.16.4