On Wed, Jul 29, 2020 at 4:17 PM Thierry Foucu <tfo...@gmail.com> wrote:
>
> On Wed, Jul 29, 2020 at 4:02 PM Matthew Szatmary <m...@szatmary.org> wrote:
>
> > Create and populate AVStream side data packet with contents of ISOBMFF
> > edit list entries
> >
> > Signed-off-by: Matthew Szatmary <m...@szatmary.org>
> > ---
> > Changelog | 1 +
> > libavcodec/packet.h | 15 +++++++++++++++
> > libavformat/mov.c | 15 +++++++++++++++
> > 3 files changed, 31 insertions(+)
> >
> > diff --git a/Changelog b/Changelog
> > index 6f648bff2b..d51e836301 100644
> > --- a/Changelog
> > +++ b/Changelog
> > @@ -10,6 +10,7 @@ version <next>:
> > - ADPCM IMA Ubisoft APM encoder
> > - Rayman 2 APM muxer
> > - AV1 encoding support SVT-AV1
> > +- AV_PKT_DATA_EDIT_LIST added to AVStream side_data
> >
> >
> > version 4.3:
> > diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> > index 0a19a0eff3..1f0e3405ed 100644
> > --- a/libavcodec/packet.h
> > +++ b/libavcodec/packet.h
> > @@ -290,6 +290,21 @@ enum AVPacketSideDataType {
> > */
> > AV_PKT_DATA_S12M_TIMECODE,
> >
> > + /**
> > + * ISO media file edit list side data packet
> > + * The structre is repeated for each entry in the edit list
> > + * The number of entries can be calculated
> > + * by dividing the total size by the entry size
> > + * Each entry is 20 bytes and is laid out as follows:
> > + * @code
> > + * s64be segment duration
> > + * s64be media time
> > + * s16be media rate
> > + * s16be media rate fraction
> > + * @endcode
> > + */
> > + AV_PKT_DATA_EDIT_LIST,
> >
>
> This seems to be really MP4 centric. Did you check if such patch sent out
> for RFC in 2018 could do it?
>
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/1522179841-34881-2-git-send-email-derek.buitenh...@gmail.com/
>
I was unaware of this patch, And it does not seem to included into master branch
>
> > +
> > /**
> > * The number of side data types.
> > * This is not part of the public API/ABI in the sense that it may
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index d16840f3df..fbfcdddf3f 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -4311,6 +4311,21 @@ static int mov_read_trak(MOVContext *c,
> > AVIOContext *pb, MOVAtom atom)
> > && sc->time_scale == st->codecpar->sample_rate) {
> > st->need_parsing = AVSTREAM_PARSE_FULL;
> > }
> > +
> > + if (sc->elst_data) {
> > + int i;
> > + uint8_t *elst_data;
> > + elst_data = av_stream_new_side_data(st,
> > AV_PKT_DATA_EDIT_LIST, sc->elst_count * 20);
> > + if (elst_data) {
> > + for (i = 0; i < sc->elst_count; i++) {
> > + uint32_t media_rate = (uint32_t)sc->elst_data[i].rate
> > * 65536.0;
> > + AV_WB64((elst_data+(i*20)), sc->elst_data[i].duration);
> > + AV_WB64((elst_data+(i*20)+8), sc->elst_data[i].time);
> > + AV_WB32((elst_data+(i*20)+16), media_rate);
> >
>
> Don't you need to know the global timescale and the stream timescale to
> make sense of those values?
>
Excellent point! I did not need to for my specific use case, But that
oversight does make this useless for many use cases.
I am resubmit a patch converting those values to AV_TIME_BASE
>
> > + }
> > + }
> > + }
> > +
> > /* Do not need those anymore. */
> > av_freep(&sc->chunk_offsets);
> > av_freep(&sc->sample_sizes);
> > --
> > 2.27.0
> >
> > On Wed, Jul 29, 2020 at 3:56 PM Matthew Szatmary <m...@szatmary.org>
> > wrote:
> > >
> > > On Wed, Jul 29, 2020 at 12:22 PM Andreas Rheinhardt
> > > <andreas.rheinha...@gmail.com> wrote:
> > > >
> > > > Matthew Szatmary:
> > > > > Create and populate AVStream side data packet with contents of
> > ISOBMFF
> > > > > edit list entries
> > > > >
> > > > > Signed-off-by: Matthew Szatmary <m...@szatmary.org>
> > > > > ---
> > > > > Changelog | 1 +
> > > > > libavcodec/packet.h | 14 ++++++++++++++
> > > > > libavformat/mov.c | 17 ++++++++++++++++-
> > > > > 3 files changed, 31 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Changelog b/Changelog
> > > > > index c37ffa82e1..2d719dd3b1 100644
> > > > > --- a/Changelog
> > > > > +++ b/Changelog
> > > > > @@ -9,6 +9,7 @@ version <next>:
> > > > > - VDPAU accelerated HEVC 10/12bit decoding
> > > > > - ADPCM IMA Ubisoft APM encoder
> > > > > - Rayman 2 APM muxer
> > > > > +- AV_PKT_DATA_EDIT_LIST added to AVStream side_data
> > > > >
> > > > >
> > > > > version 4.3:
> > > > > diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> > > > > index 0a19a0eff3..5faa594cf5 100644
> > > > > --- a/libavcodec/packet.h
> > > > > +++ b/libavcodec/packet.h
> > > > > @@ -290,6 +290,20 @@ enum AVPacketSideDataType {
> > > > > */
> > > > > AV_PKT_DATA_S12M_TIMECODE,
> > > > >
> > > > > + /**
> > > > > + * ISO media file edit list side data packet
> > > > > + * The structure is repeated for each entry in the edit list
> > > > > + * The number of entries can be calculated
> > > > > + * by dividing the packet size by the entry size
> > > > > + * Each entry is 20 bytes and is laid out as follows:
> > > > > + * @code
> > > > > + * s64le duration
> > > > > + * s64le time
> > > > > + * float32le rate
> > > Good point, I will make that change.
> > >
> > > >
> > > > You are obviously copying the MOVElst structure; yet the rate is a
> > 16.16
> > > > fixed point number in the file and not a float, so one should probably
> > > > use this.
> > > >
> > > > > + * @endcode
> > > > > + */
> > > > > + AV_PKT_DATA_EDIT_LIST,
> > > > > +
> > > > > /**
> > > > > * The number of side data types.
> > > > > * This is not part of the public API/ABI in the sense that it
> > may
> > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > > > index d16840f3df..bb2c940e80 100644
> > > > > --- a/libavformat/mov.c
> > > > > +++ b/libavformat/mov.c
> > > > > @@ -4317,7 +4317,6 @@ static int mov_read_trak(MOVContext *c,
> > > > > AVIOContext *pb, MOVAtom atom)
> > > > > av_freep(&sc->keyframes);
> > > > > av_freep(&sc->stts_data);
> > > > > av_freep(&sc->stps_data);
> > > > > - av_freep(&sc->elst_data);
> > > >
> > > > This is still needed, namely if an error happens before you can attach
> > > > the stream side-data. E.g. if an invalid edit list is found and
> > > > standards compliance is set to strict. Or if av_stream_new_side_data()
> > > > fails.
> > >
> > > av_freep(&sc->elst_data); is also called in mov_read_close, So it
> > > would only leak if the API was used incorrectly. But that said, I
> > > think I can move the logic to mov_read_trak, and make the whole point
> > > moot. I was just trying to keep it near the other side_data calls.
> > >
> > > >
> > > > > av_freep(&sc->rap_group);
> > > > >
> > > > > return 0;
> > > > > @@ -7662,6 +7661,22 @@ static int mov_read_header(AVFormatContext *s)
> > > > > AVStream *st = s->streams[i];
> > > > > MOVStreamContext *sc = st->priv_data;
> > > > >
> > > > > + if (sc->elst_data) {
> > > > > + uint8_t *elst_data;
> > > > > + elst_data = av_stream_new_side_data(st,
> > > > > AV_PKT_DATA_EDIT_LIST, sc->elst_count * 20);
> > > >
> > > > I wonder whether it would be advantageouos to use
> > > > av_stream_add_side_data() here.
> > >
> > > av_stream_new_side_data is just a wrapper for av_malloc +
> > > av_stream_add_side_data
> > >
> > > > > +
> > > > > + if (!elst_data)
> > > > > + goto fail;
> > > > > +
> > > > > + for (j = 0; j < sc->elst_count; j++) {
> > > > > + AV_WB64((elst_data+(j*20)),
> > sc->elst_data[j].duration);
> > > > > + AV_WB64((elst_data+(j*20)+8),
> > sc->elst_data[j].time);
> > > >
> > > > "WB" stands for "Write Big-endian", yet your documentation says that it
> > > > is supposed to be little-endian.
> > >
> > > thanks, new patch included
> > >
> > >
> > > >
> > > > > + AV_WB32((elst_data+(j*20)+16),
> > sc->elst_data[j].rate);
> > > > > + }
> > > > > +
> > > > > + av_freep(&sc->elst_data);
> > > > > + }
> > > > > +
> > > > > switch (st->codecpar->codec_type) {
> > > > > case AVMEDIA_TYPE_AUDIO:
> > > > > err = ff_replaygain_export(st, s->metadata);
> > > > >
> > > >
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel@ffmpeg.org
> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >
> > > > To unsubscribe, visit link above, or email
> > > > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
> > >
> > >
> > > Create and populate AVStream side data packet with contents of ISOBMFF
> > > edit list entries
> > >
> > > Signed-off-by: Matthew Szatmary <m...@szatmary.org>
> > > ---
> > > Changelog | 1 +
> > > libavcodec/packet.h | 15 +++++++++++++++
> > > libavformat/mov.c | 15 +++++++++++++++
> > > 3 files changed, 31 insertions(+)
> > >
> > > diff --git a/Changelog b/Changelog
> > > index 6f648bff2b..d51e836301 100644
> > > --- a/Changelog
> > > +++ b/Changelog
> > > @@ -10,6 +10,7 @@ version <next>:
> > > - ADPCM IMA Ubisoft APM encoder
> > > - Rayman 2 APM muxer
> > > - AV1 encoding support SVT-AV1
> > > +- AV_PKT_DATA_EDIT_LIST added to AVStream side_data
> > >
> > >
> > > version 4.3:
> > > diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> > > index 0a19a0eff3..1f0e3405ed 100644
> > > --- a/libavcodec/packet.h
> > > +++ b/libavcodec/packet.h
> > > @@ -290,6 +290,21 @@ enum AVPacketSideDataType {
> > > */
> > > AV_PKT_DATA_S12M_TIMECODE,
> > >
> > > + /**
> > > + * ISO media file edit list side data packet
> > > + * The structure is repeated for each entry in the edit list
> > > + * The number of entries can be calculated
> > > + * by dividing the packet size by the entry size
> > > + * Each entry is 20 bytes and is laid out as follows:
> > > + * @code
> > > + * s64be segment duration
> > > + * s64be media time
> > > + * s16be media rate
> > > + * s16be media rate fraction
> > > + * @endcode
> > > + */
> > > + AV_PKT_DATA_EDIT_LIST,
> > > +
> > > /**
> > > * The number of side data types.
> > > * This is not part of the public API/ABI in the sense that it may
> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > index d16840f3df..fbfcdddf3f 100644
> > > --- a/libavformat/mov.c
> > > +++ b/libavformat/mov.c
> > > @@ -4311,6 +4311,21 @@ static int mov_read_trak(MOVContext *c,
> > > AVIOContext *pb, MOVAtom atom)
> > > && sc->time_scale == st->codecpar->sample_rate) {
> > > st->need_parsing = AVSTREAM_PARSE_FULL;
> > > }
> > > +
> > > + if (sc->elst_data) {
> > > + int i;
> > > + uint8_t *elst_data;
> > > + elst_data = av_stream_new_side_data(st,
> > > AV_PKT_DATA_EDIT_LIST, sc->elst_count * 20);
> > > + if (elst_data) {
> > > + for (i = 0; i < sc->elst_count; i++) {
> > > + uint32_t media_rate =
> > > (uint32_t)(sc->elst_data[i].rate * 65536.0);
> > > + AV_WB64((elst_data+(i*20)), sc->elst_data[i].duration);
> > > + AV_WB64((elst_data+(i*20)+8), sc->elst_data[i].time);
> > > + AV_WB32((elst_data+(i*20)+16), media_rate);
> > > + }
> > > + }
> > > + }
> > > +
> > > /* Do not need those anymore. */
> > > av_freep(&sc->chunk_offsets);
> > > av_freep(&sc->sample_sizes);
> > > --
> > > 2.27.0
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
>
>
> --
>
> Thierry Foucu
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
