diff mbox series

[FFmpeg-devel] Create and populate AVStream side data packet with contents of ISOBMFF edit list entries

Message ID CAEUW-_qfbRFVAXHLSMF5sNDcN5MO-XP-MBxGHtQfpk4=oDMw-w@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel] Create and populate AVStream side data packet with contents of ISOBMFF edit list entries | expand

Checks

Context Check Description
andriy/default pending
andriy/configure warning Failed to apply patch

Commit Message

Matthew Szatmary July 29, 2020, 6:25 p.m. UTC
Create and populate AVStream side data packet with contents of ISOBMFF
edit list entries

Signed-off-by: Matthew Szatmary <matt@szatmary.org>
---
 Changelog           |  1 +
 libavcodec/packet.h | 14 ++++++++++++++
 libavformat/mov.c   | 17 ++++++++++++++++-
 3 files changed, 31 insertions(+), 1 deletion(-)

     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);
+
+            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);
+                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);

Comments

Andreas Rheinhardt July 29, 2020, 7:22 p.m. UTC | #1
Matthew Szatmary:
> Create and populate AVStream side data packet with contents of ISOBMFF
> edit list entries
> 
> Signed-off-by: Matthew Szatmary <matt@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

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

> +
> +            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.

> +                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);
>
Matthew Szatmary July 29, 2020, 10:56 p.m. UTC | #2
On Wed, Jul 29, 2020 at 12:22 PM Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
>
> Matthew Szatmary:
> > Create and populate AVStream side data packet with contents of ISOBMFF
> > edit list entries
> >
> > Signed-off-by: Matthew Szatmary <matt@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-request@ffmpeg.org with subject "unsubscribe".


Create and populate AVStream side data packet with contents of ISOBMFF
edit list entries

Signed-off-by: Matthew Szatmary <matt@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);
Matthew Szatmary July 29, 2020, 11:01 p.m. UTC | #3
Create and populate AVStream side data packet with contents of ISOBMFF
edit list entries

Signed-off-by: Matthew Szatmary <matt@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,
+
     /**
      * 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);
Thierry Foucu July 29, 2020, 11:16 p.m. UTC | #4
On Wed, Jul 29, 2020 at 4:02 PM Matthew Szatmary <matt@szatmary.org> wrote:

> Create and populate AVStream side data packet with contents of ISOBMFF
> edit list entries
>
> Signed-off-by: Matthew Szatmary <matt@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.buitenhuis@gmail.com/


> +
>      /**
>       * 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?


> +            }
> +        }
> +    }
> +
>      /* 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 <matt@szatmary.org>
> wrote:
> >
> > On Wed, Jul 29, 2020 at 12:22 PM Andreas Rheinhardt
> > <andreas.rheinhardt@gmail.com> wrote:
> > >
> > > Matthew Szatmary:
> > > > Create and populate AVStream side data packet with contents of
> ISOBMFF
> > > > edit list entries
> > > >
> > > > Signed-off-by: Matthew Szatmary <matt@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-request@ffmpeg.org with subject "unsubscribe".
> >
> >
> > Create and populate AVStream side data packet with contents of ISOBMFF
> > edit list entries
> >
> > Signed-off-by: Matthew Szatmary <matt@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-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt July 30, 2020, 4:35 a.m. UTC | #5
Matthew Szatmary:
> On Wed, Jul 29, 2020 at 12:22 PM Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com> wrote:
>>
>> Matthew Szatmary:
>>> Create and populate AVStream side data packet with contents of ISOBMFF
>>> edit list entries
>>>
>>> Signed-off-by: Matthew Szatmary <matt@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.
> 

Sorry, I thought you were removing the av_freep() from mov_read_close();
I didn't realize that it would also be freed in mov_read_trak().

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

Yes, and as such I hoped that it could be used to avoid the allocation;
but it is impossible: The MOVElst struct will have padding at the end so
that its size is 24 (ordinarily; compilers are free to insert even more
padding), so that it can't be reused anyway.

> 
>>> +
>>> +            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-request@ffmpeg.org with subject "unsubscribe".
> 
> 
> Create and populate AVStream side data packet with contents of ISOBMFF
> edit list entries
> 
> Signed-off-by: Matthew Szatmary <matt@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);
>
Matthew Szatmary July 30, 2020, 3:55 p.m. UTC | #6
On Wed, Jul 29, 2020 at 4:17 PM Thierry Foucu <tfoucu@gmail.com> wrote:
>
> On Wed, Jul 29, 2020 at 4:02 PM Matthew Szatmary <matt@szatmary.org> wrote:
>
> > Create and populate AVStream side data packet with contents of ISOBMFF
> > edit list entries
> >
> > Signed-off-by: Matthew Szatmary <matt@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.buitenhuis@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 <matt@szatmary.org>
> > wrote:
> > >
> > > On Wed, Jul 29, 2020 at 12:22 PM Andreas Rheinhardt
> > > <andreas.rheinhardt@gmail.com> wrote:
> > > >
> > > > Matthew Szatmary:
> > > > > Create and populate AVStream side data packet with contents of
> > ISOBMFF
> > > > > edit list entries
> > > > >
> > > > > Signed-off-by: Matthew Szatmary <matt@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-request@ffmpeg.org with subject "unsubscribe".
> > >
> > >
> > > Create and populate AVStream side data packet with contents of ISOBMFF
> > > edit list entries
> > >
> > > Signed-off-by: Matthew Szatmary <matt@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-request@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-request@ffmpeg.org with subject "unsubscribe".
Matthew Szatmary July 30, 2020, 4:03 p.m. UTC | #7
From 91ae719dd69cba5153e4ce1aad2eb3ce819668b1 Mon Sep 17 00:00:00 2001
From: Matthew Szatmary <matt@szatmary.org>
Date: Thu, 23 Jul 2020 16:36:49 -0700
Subject: [PATCH] Create and populate AVStream side data packet with contents
 of ISOBMFF edit list entries

Signed-off-by: Matthew Szatmary <matt@szatmary.org>
---
 Changelog           |  1 +
 libavcodec/packet.h | 15 +++++++++++++++
 libavformat/mov.c   | 19 +++++++++++++++++++
 3 files changed, 35 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..f4a00ccf1e 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 in AV_TIME_BASE
+     * s64be media time in AV_TIME_BASE
+     * 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..0d29588831 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4311,6 +4311,25 @@ 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;
+                int64_t duration, time;
+                media_rate = (uint32_t)(sc->elst_data[i].rate * 65536.0);
+                duration = av_rescale(sc->elst_data[i].duration,
c->time_scale, AV_TIME_BASE);
+                time = av_rescale(sc->elst_data[i].time,
c->time_scale, AV_TIME_BASE);
+                AV_WB64((elst_data+(i*20)), duration);
+                AV_WB64((elst_data+(i*20)+8), 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);
Derek Buitenhuis Aug. 3, 2020, 3:51 p.m. UTC | #8
On 30/07/2020 16:55, Matthew Szatmary wrote:
>> 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.buitenhuis@gmail.com/
>>
> I was unaware of this patch, And it does not seem to included into master branch

Yeah, that kind of just... dropped off my radar and a I never sent a v2.

I don't really really have a strong opinion on whether to pick that 2 year old
patch up again, or continue with this current patch from Matthew.

I don't really think it matters, as long as anything pushed isn't too rigid to
extend to use with ordered chapters or other cosmic horrors involving MXF.

- Derek
diff mbox series

Patch

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
+     * @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);
     av_freep(&sc->rap_group);