Re: [FFmpeg-devel] [PATCH v2 5/5] lavf/demux: duplicate side_data in parse_packet()

2024-03-01 Thread James Almer 

On 3/1/2024 10:49 AM, James Almer wrote:

On 3/1/2024 10:39 AM, Nicolas Gaullier wrote:

Signed-off-by: Nicolas Gaullier 
---
  libavformat/demux.c   |  23 ++-
  tests/ref/fate/concat-demuxer-simple2-lavf-ts | 164 +-
  tests/ref/fate/ts-demux   |   8 +-
  3 files changed, 104 insertions(+), 91 deletions(-)

diff --git a/libavformat/demux.c b/libavformat/demux.c
index 2e1d0ed66d..722bb35c4c 100644
--- a/libavformat/demux.c
+++ b/libavformat/demux.c
@@ -1186,7 +1186,7 @@ static int parse_packet(AVFormatContext *s, 
AVPacket *pkt,

  FFStream *const sti = ffstream(st);
  const uint8_t *data = pkt->data;
  int size = pkt->size;
-    int ret = 0, got_output = flush;
+    int ret = 0, got_output = flush, pkt_side_data_consumed = 0;
  if (!size && !flush && sti->parser->flags & 
PARSER_FLAG_COMPLETE_FRAMES) {

  // preserve 0-size sync packets
@@ -1231,10 +1231,19 @@ static int parse_packet(AVFormatContext *s, 
AVPacket *pkt,

  }
  if (pkt->side_data) {
-    out_pkt->side_data   = pkt->side_data;
-    out_pkt->side_data_elems = pkt->side_data_elems;
-    pkt->side_data  = NULL;
-    pkt->side_data_elems    = 0;
+    if (!pkt_side_data_consumed) {


Can't you just check for out_pkt->side_data instead?


Nevermind, i misread the code.

Please add a comment about how pkt_side_data_consumed prevents copying 
side data for the first (and potentially only) output iteration.





+    out_pkt->side_data   = pkt->side_data;
+    out_pkt->side_data_elems = pkt->side_data_elems;
+    pkt_side_data_consumed = 1;
+    } else for (int i = 0; i < pkt->side_data_elems; i++) {
+    const AVPacketSideData *const src_sd = 
>side_data[i];
+    uint8_t *dst_data = av_packet_new_side_data(out_pkt, 
src_sd->type, src_sd->size);

+    if (!dst_data) {
+    ret = AVERROR(ENOMEM);
+    goto fail;
+    }
+    memcpy(dst_data, src_sd->data, src_sd->size);
+    }
  }
  /* set the duration */
@@ -1286,6 +1295,10 @@ static int parse_packet(AVFormatContext *s, 
AVPacket *pkt,

  }
  fail:
+    if (pkt_side_data_consumed) {
+    pkt->side_data  = NULL;
+    pkt->side_data_elems    = 0;
+    }
  if (ret < 0)
  av_packet_unref(out_pkt);
  av_packet_unref(pkt);
diff --git a/tests/ref/fate/concat-demuxer-simple2-lavf-ts 
b/tests/ref/fate/concat-demuxer-simple2-lavf-ts

index 548cab01c6..ee49e331f3 100644
--- a/tests/ref/fate/concat-demuxer-simple2-lavf-ts
+++ b/tests/ref/fate/concat-demuxer-simple2-lavf-ts
@@ -7,19 +7,19 @@ 
video|1|18982|0.210911|15382|0.170911|3600|0.04|13092|84788|___|MPEGTS Strea
  
video|1|22582|0.250911|18982|0.210911|3600|0.04|12755|98700|___|MPEGTS Stream ID|224
  
video|1|26182|0.290911|22582|0.250911|3600|0.04|12023|111860|___|MPEGTS Stream ID|224
  audio|0|0|0.00|0|0.00|2351|0.026122|208|152844|K__|MPEGTS 
Stream ID|192

-audio|0|2351|0.026122|2351|0.026122|2351|0.026122|209|N/A|K__
-audio|0|4702|0.052244|4702|0.052244|2351|0.026122|209|N/A|K__
-audio|0|7053|0.078367|7053|0.078367|2351|0.026122|209|N/A|K__
-audio|0|9404|0.104489|9404|0.104489|2351|0.026122|209|N/A|K__
-audio|0|11755|0.130611|11755|0.130611|2351|0.026122|209|N/A|K__
-audio|0|14106|0.156733|14106|0.156733|2351|0.026122|209|N/A|K__
-audio|0|16457|0.182856|16457|0.182856|2351|0.026122|209|N/A|K__
-audio|0|18808|0.208978|18808|0.208978|2351|0.026122|209|N/A|K__
-audio|0|21159|0.235100|21159|0.235100|2351|0.026122|209|N/A|K__
-audio|0|23510|0.261222|23510|0.261222|2351|0.026122|209|N/A|K__
-audio|0|25861|0.287344|25861|0.287344|2351|0.026122|209|N/A|K__
-audio|0|28212|0.313467|28212|0.313467|2351|0.026122|209|N/A|K__
-audio|0|30563|0.339589|30563|0.339589|2351|0.026122|209|N/A|K__
+audio|0|2351|0.026122|2351|0.026122|2351|0.026122|209|N/A|K__|MPEGTS 
Stream ID|192
+audio|0|4702|0.052244|4702|0.052244|2351|0.026122|209|N/A|K__|MPEGTS 
Stream ID|192
+audio|0|7053|0.078367|7053|0.078367|2351|0.026122|209|N/A|K__|MPEGTS 
Stream ID|192
+audio|0|9404|0.104489|9404|0.104489|2351|0.026122|209|N/A|K__|MPEGTS 
Stream ID|192

+audio|0|11755|0.130611|11755|0.130611|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|14106|0.156733|14106|0.156733|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|16457|0.182856|16457|0.182856|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|18808|0.208978|18808|0.208978|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|21159|0.235100|21159|0.235100|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|23510|0.261222|23510|0.261222|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|25861|0.287344|25861|0.287344|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|28212|0.313467|28212|0.313467|2351|0.026122|209|N/A|K__|MPEGTS Stream

Re: [FFmpeg-devel] [PATCH v2 5/5] lavf/demux: duplicate side_data in parse_packet()

2024-03-01 Thread James Almer 

On 3/1/2024 10:39 AM, Nicolas Gaullier wrote:

Signed-off-by: Nicolas Gaullier 
---
  libavformat/demux.c   |  23 ++-
  tests/ref/fate/concat-demuxer-simple2-lavf-ts | 164 +-
  tests/ref/fate/ts-demux   |   8 +-
  3 files changed, 104 insertions(+), 91 deletions(-)

diff --git a/libavformat/demux.c b/libavformat/demux.c
index 2e1d0ed66d..722bb35c4c 100644
--- a/libavformat/demux.c
+++ b/libavformat/demux.c
@@ -1186,7 +1186,7 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt,
  FFStream *const sti = ffstream(st);
  const uint8_t *data = pkt->data;
  int size = pkt->size;
-int ret = 0, got_output = flush;
+int ret = 0, got_output = flush, pkt_side_data_consumed = 0;
  
  if (!size && !flush && sti->parser->flags & PARSER_FLAG_COMPLETE_FRAMES) {

  // preserve 0-size sync packets
@@ -1231,10 +1231,19 @@ static int parse_packet(AVFormatContext *s, AVPacket 
*pkt,
  }
  
  if (pkt->side_data) {

-out_pkt->side_data   = pkt->side_data;
-out_pkt->side_data_elems = pkt->side_data_elems;
-pkt->side_data  = NULL;
-pkt->side_data_elems= 0;
+if (!pkt_side_data_consumed) {


Can't you just check for out_pkt->side_data instead?


+out_pkt->side_data   = pkt->side_data;
+out_pkt->side_data_elems = pkt->side_data_elems;
+pkt_side_data_consumed = 1;
+} else for (int i = 0; i < pkt->side_data_elems; i++) {
+const AVPacketSideData *const src_sd = >side_data[i];
+uint8_t *dst_data = av_packet_new_side_data(out_pkt, 
src_sd->type, src_sd->size);
+if (!dst_data) {
+ret = AVERROR(ENOMEM);
+goto fail;
+}
+memcpy(dst_data, src_sd->data, src_sd->size);
+}
  }
  
  /* set the duration */

@@ -1286,6 +1295,10 @@ static int parse_packet(AVFormatContext *s, AVPacket 
*pkt,
  }
  
  fail:

+if (pkt_side_data_consumed) {
+pkt->side_data  = NULL;
+pkt->side_data_elems= 0;
+}
  if (ret < 0)
  av_packet_unref(out_pkt);
  av_packet_unref(pkt);
diff --git a/tests/ref/fate/concat-demuxer-simple2-lavf-ts 
b/tests/ref/fate/concat-demuxer-simple2-lavf-ts
index 548cab01c6..ee49e331f3 100644
--- a/tests/ref/fate/concat-demuxer-simple2-lavf-ts
+++ b/tests/ref/fate/concat-demuxer-simple2-lavf-ts
@@ -7,19 +7,19 @@ 
video|1|18982|0.210911|15382|0.170911|3600|0.04|13092|84788|___|MPEGTS Strea
  video|1|22582|0.250911|18982|0.210911|3600|0.04|12755|98700|___|MPEGTS 
Stream ID|224
  video|1|26182|0.290911|22582|0.250911|3600|0.04|12023|111860|___|MPEGTS 
Stream ID|224
  audio|0|0|0.00|0|0.00|2351|0.026122|208|152844|K__|MPEGTS Stream 
ID|192
-audio|0|2351|0.026122|2351|0.026122|2351|0.026122|209|N/A|K__
-audio|0|4702|0.052244|4702|0.052244|2351|0.026122|209|N/A|K__
-audio|0|7053|0.078367|7053|0.078367|2351|0.026122|209|N/A|K__
-audio|0|9404|0.104489|9404|0.104489|2351|0.026122|209|N/A|K__
-audio|0|11755|0.130611|11755|0.130611|2351|0.026122|209|N/A|K__
-audio|0|14106|0.156733|14106|0.156733|2351|0.026122|209|N/A|K__
-audio|0|16457|0.182856|16457|0.182856|2351|0.026122|209|N/A|K__
-audio|0|18808|0.208978|18808|0.208978|2351|0.026122|209|N/A|K__
-audio|0|21159|0.235100|21159|0.235100|2351|0.026122|209|N/A|K__
-audio|0|23510|0.261222|23510|0.261222|2351|0.026122|209|N/A|K__
-audio|0|25861|0.287344|25861|0.287344|2351|0.026122|209|N/A|K__
-audio|0|28212|0.313467|28212|0.313467|2351|0.026122|209|N/A|K__
-audio|0|30563|0.339589|30563|0.339589|2351|0.026122|209|N/A|K__
+audio|0|2351|0.026122|2351|0.026122|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|4702|0.052244|4702|0.052244|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|7053|0.078367|7053|0.078367|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|9404|0.104489|9404|0.104489|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|11755|0.130611|11755|0.130611|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|14106|0.156733|14106|0.156733|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|16457|0.182856|16457|0.182856|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|18808|0.208978|18808|0.208978|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|21159|0.235100|21159|0.235100|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|23510|0.261222|23510|0.261222|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|25861|0.287344|25861|0.287344|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|28212|0.313467|28212|0.313467|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|30563|0.339589|30563|0.339589|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
  video|1|29782|0.330911|26182|0.290911|3600|0.04|14098|124268|___|MPEGTS 
Stream ID|224

[FFmpeg-devel] [PATCH v2 5/5] lavf/demux: duplicate side_data in parse_packet()

2024-03-01 Thread Nicolas Gaullier 
Signed-off-by: Nicolas Gaullier 
---
 libavformat/demux.c   |  23 ++-
 tests/ref/fate/concat-demuxer-simple2-lavf-ts | 164 +-
 tests/ref/fate/ts-demux   |   8 +-
 3 files changed, 104 insertions(+), 91 deletions(-)

diff --git a/libavformat/demux.c b/libavformat/demux.c
index 2e1d0ed66d..722bb35c4c 100644
--- a/libavformat/demux.c
+++ b/libavformat/demux.c
@@ -1186,7 +1186,7 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt,
 FFStream *const sti = ffstream(st);
 const uint8_t *data = pkt->data;
 int size = pkt->size;
-int ret = 0, got_output = flush;
+int ret = 0, got_output = flush, pkt_side_data_consumed = 0;
 
 if (!size && !flush && sti->parser->flags & PARSER_FLAG_COMPLETE_FRAMES) {
 // preserve 0-size sync packets
@@ -1231,10 +1231,19 @@ static int parse_packet(AVFormatContext *s, AVPacket 
*pkt,
 }
 
 if (pkt->side_data) {
-out_pkt->side_data   = pkt->side_data;
-out_pkt->side_data_elems = pkt->side_data_elems;
-pkt->side_data  = NULL;
-pkt->side_data_elems= 0;
+if (!pkt_side_data_consumed) {
+out_pkt->side_data   = pkt->side_data;
+out_pkt->side_data_elems = pkt->side_data_elems;
+pkt_side_data_consumed = 1;
+} else for (int i = 0; i < pkt->side_data_elems; i++) {
+const AVPacketSideData *const src_sd = >side_data[i];
+uint8_t *dst_data = av_packet_new_side_data(out_pkt, 
src_sd->type, src_sd->size);
+if (!dst_data) {
+ret = AVERROR(ENOMEM);
+goto fail;
+}
+memcpy(dst_data, src_sd->data, src_sd->size);
+}
 }
 
 /* set the duration */
@@ -1286,6 +1295,10 @@ static int parse_packet(AVFormatContext *s, AVPacket 
*pkt,
 }
 
 fail:
+if (pkt_side_data_consumed) {
+pkt->side_data  = NULL;
+pkt->side_data_elems= 0;
+}
 if (ret < 0)
 av_packet_unref(out_pkt);
 av_packet_unref(pkt);
diff --git a/tests/ref/fate/concat-demuxer-simple2-lavf-ts 
b/tests/ref/fate/concat-demuxer-simple2-lavf-ts
index 548cab01c6..ee49e331f3 100644
--- a/tests/ref/fate/concat-demuxer-simple2-lavf-ts
+++ b/tests/ref/fate/concat-demuxer-simple2-lavf-ts
@@ -7,19 +7,19 @@ 
video|1|18982|0.210911|15382|0.170911|3600|0.04|13092|84788|___|MPEGTS Strea
 video|1|22582|0.250911|18982|0.210911|3600|0.04|12755|98700|___|MPEGTS 
Stream ID|224
 video|1|26182|0.290911|22582|0.250911|3600|0.04|12023|111860|___|MPEGTS 
Stream ID|224
 audio|0|0|0.00|0|0.00|2351|0.026122|208|152844|K__|MPEGTS Stream ID|192
-audio|0|2351|0.026122|2351|0.026122|2351|0.026122|209|N/A|K__
-audio|0|4702|0.052244|4702|0.052244|2351|0.026122|209|N/A|K__
-audio|0|7053|0.078367|7053|0.078367|2351|0.026122|209|N/A|K__
-audio|0|9404|0.104489|9404|0.104489|2351|0.026122|209|N/A|K__
-audio|0|11755|0.130611|11755|0.130611|2351|0.026122|209|N/A|K__
-audio|0|14106|0.156733|14106|0.156733|2351|0.026122|209|N/A|K__
-audio|0|16457|0.182856|16457|0.182856|2351|0.026122|209|N/A|K__
-audio|0|18808|0.208978|18808|0.208978|2351|0.026122|209|N/A|K__
-audio|0|21159|0.235100|21159|0.235100|2351|0.026122|209|N/A|K__
-audio|0|23510|0.261222|23510|0.261222|2351|0.026122|209|N/A|K__
-audio|0|25861|0.287344|25861|0.287344|2351|0.026122|209|N/A|K__
-audio|0|28212|0.313467|28212|0.313467|2351|0.026122|209|N/A|K__
-audio|0|30563|0.339589|30563|0.339589|2351|0.026122|209|N/A|K__
+audio|0|2351|0.026122|2351|0.026122|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|4702|0.052244|4702|0.052244|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|7053|0.078367|7053|0.078367|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|9404|0.104489|9404|0.104489|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|11755|0.130611|11755|0.130611|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|14106|0.156733|14106|0.156733|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|16457|0.182856|16457|0.182856|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|18808|0.208978|18808|0.208978|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|21159|0.235100|21159|0.235100|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|23510|0.261222|23510|0.261222|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|25861|0.287344|25861|0.287344|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|28212|0.313467|28212|0.313467|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
+audio|0|30563|0.339589|30563|0.339589|2351|0.026122|209|N/A|K__|MPEGTS Stream 
ID|192
 video|1|29782|0.330911|26182|0.290911|3600|0.04|14098|124268|___|MPEGTS 
Stream ID|224
 video|1|33382|0.370911|29782|0.330911|3600|0.04|13329|139120|___|MPEGTS 
Stream ID|224