Re: [FFmpeg-devel] [PATCH 17/18] lavf/dv: do not update AVCodecParameters.sample_rate while demuxing

2022-08-30 Thread Anton Khirnov 
Quoting Andreas Rheinhardt (2022-08-24 16:20:35)
> Anton Khirnov:
> > Demuxers are not allowed to do this and few callers, if any, will handle
> > this correctly. Send the AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE side data
> > instead.
> 
> 1. One of the callers which did not handle this correctly is the mov
> demuxer: A sample rate update would not propagate to the user, because
> it uses a separate AVFormatContext for it. This should be mentioned in
> the commit message. (Btw: I don't see anything that guarantees that the
> samplerate and timebase of the inner demuxer and the sample rate and
> time base reported to the user (presumably taken from mov structures)
> coincide. Given that dv_fctx and dv_demux are part of MOVContext, they
> would also leak if it would be attempted to allocate them multiple
> times. Do you see anything that guarantees that they will not be
> allocated multiple times?)
> 2. In case of ff_add_param_change() failure, users will just interpret
> that as "no more packets available", won't they?

yes, I think so

> This might be wrong, but it is not problematic.

I agree

> 3. Have you tested this with a sample with actual parameter changes?

Yes, I made a sample by concatenating two dv files. The side data is
exported correctly, but the PCM decoder fails because it is not marked
with AV_CODEC_CAP_PARAM_CHANGE. Patches welcome.

-- 
Anton Khirnov
___
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".

Re: [FFmpeg-devel] [PATCH 17/18] lavf/dv: do not update AVCodecParameters.sample_rate while demuxing

2022-08-24 Thread Andreas Rheinhardt 
Anton Khirnov:
> Demuxers are not allowed to do this and few callers, if any, will handle
> this correctly. Send the AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE side data
> instead.
> ---
>  libavformat/dv.c | 49 +---
>  1 file changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/libavformat/dv.c b/libavformat/dv.c
> index 9c8b0a262c..ffed1a7a90 100644
> --- a/libavformat/dv.c
> +++ b/libavformat/dv.c
> @@ -33,6 +33,7 @@
>  
>  #include 
>  #include "avformat.h"
> +#include "demux.h"
>  #include "internal.h"
>  #include "libavcodec/dv_profile.h"
>  #include "libavcodec/dv.h"
> @@ -46,7 +47,7 @@
>  #if CONFIG_DV_DEMUXER
>  
>  // Must be kept in sync with AVPacket
> -struct DVPacket {
> +typedef struct DVPacket {
>  int64_t  pts;
>  uint8_t *data;
>  int  size;
> @@ -54,7 +55,10 @@ struct DVPacket {
>  int  flags;
>  int64_t  pos;
>  int64_t  duration;
> -};
> +
> +int sample_rate;
> +int last_sample_rate;
> +} DVPacket;
>  
>  struct DVDemuxContext {
>  const AVDVProfile*  sys;/* Current DV profile. E.g.: 525/60, 625/50 
> */
> @@ -237,7 +241,7 @@ static int dv_extract_audio(const uint8_t *frame, uint8_t 
> **ppcm,
>  static int dv_extract_audio_info(DVDemuxContext *c, const uint8_t *frame)
>  {
>  const uint8_t *as_pack;
> -int freq, stype, smpls, quant, i, ach;
> +int freq, stype, smpls, quant, i, ach, sr;
>  
>  as_pack = dv_extract_pack(frame, DV_AUDIO_SOURCE);
>  if (!as_pack || !c->sys) {/* No audio ? */
> @@ -255,6 +259,7 @@ static int dv_extract_audio_info(DVDemuxContext *c, const 
> uint8_t *frame)
> "Unrecognized audio sample rate index (%d)\n", freq);
>  return 0;
>  }
> +sr = dv_audio_frequency[freq];
>  
>  if (stype > 3) {
>  av_log(c->fctx, AV_LOG_ERROR, "stype %d is invalid\n", stype);
> @@ -280,7 +285,10 @@ static int dv_extract_audio_info(DVDemuxContext *c, 
> const uint8_t *frame)
>  c->ast[i]->codecpar->codec_id   = AV_CODEC_ID_PCM_S16LE;
>  c->ast[i]->codecpar->ch_layout  = 
> (AVChannelLayout)AV_CHANNEL_LAYOUT_STEREO;
>  c->ast[i]->start_time   = 0;
> -c->ast[i]->codecpar->bit_rate   = 2 * dv_audio_frequency[freq] * 
> 16;
> +c->ast[i]->codecpar->bit_rate   = 2 * sr * 16;
> +
> +c->ast[i]->codecpar->sample_rate = sr;
> +c->audio_pkt[i].last_sample_rate = sr;
>  
>  c->audio_pkt[i].size = 0;
>  c->audio_pkt[i].data = c->audio_buf[i];
> @@ -290,7 +298,8 @@ static int dv_extract_audio_info(DVDemuxContext *c, const 
> uint8_t *frame)
>  c->audio_pkt[i].duration = 0;
>  c->audio_pkt[i].pos  = -1;
>  }
> -c->ast[i]->codecpar->sample_rate= dv_audio_frequency[freq];
> +
> +c->audio_pkt[i].sample_rate = sr;
>  }
>  c->ach = ach;
>  
> @@ -380,16 +389,26 @@ int avpriv_dv_get_packet(DVDemuxContext *c, AVPacket 
> *pkt)
>  
>  for (i = 0; i < c->ach; i++) {
>  if (c->ast[i] && c->audio_pkt[i].size) {
> -pkt->size = c->audio_pkt[i].size;
> -pkt->data = c->audio_pkt[i].data;
> -pkt->stream_index = c->audio_pkt[i].stream_index;
> -pkt->flags= c->audio_pkt[i].flags;
> -pkt->pts  = c->audio_pkt[i].pts;
> -pkt->duration = c->audio_pkt[i].duration;
> -pkt->pos  = c->audio_pkt[i].pos;
> -
> -c->audio_pkt[i].size = 0;
> -size = pkt->size;
> +DVPacket *dpkt = >audio_pkt[i];
> +
> +pkt->size = dpkt->size;
> +pkt->data = dpkt->data;
> +pkt->stream_index = dpkt->stream_index;
> +pkt->flags= dpkt->flags;
> +pkt->pts  = dpkt->pts;
> +pkt->duration = dpkt->duration;
> +pkt->pos  = dpkt->pos;
> +
> +dpkt->size = 0;
> +size   = pkt->size;
> +
> +if (dpkt->last_sample_rate != dpkt->sample_rate) {
> +int ret = ff_add_param_change(pkt, 0, 0, dpkt->sample_rate, 
> 0, 0);
> +if (ret < 0)
> +return ret;
> +dpkt->last_sample_rate = dpkt->sample_rate;
> +}
> +
>  break;
>  }
>  }

1. One of the callers which did not handle this correctly is the mov
demuxer: A sample rate update would not propagate to the user, because
it uses a separate AVFormatContext for it. This should be mentioned in
the commit message. (Btw: I don't see anything that guarantees that the
samplerate and timebase of the inner demuxer and the sample rate and
time base reported to the user (presumably taken from mov structures)
coincide. Given that dv_fctx and dv_demux are part of MOVContext, they
would also leak if it would be attempted to

[FFmpeg-devel] [PATCH 17/18] lavf/dv: do not update AVCodecParameters.sample_rate while demuxing

2022-08-24 Thread Anton Khirnov 
Demuxers are not allowed to do this and few callers, if any, will handle
this correctly. Send the AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE side data
instead.
---
 libavformat/dv.c | 49 +---
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/libavformat/dv.c b/libavformat/dv.c
index 9c8b0a262c..ffed1a7a90 100644
--- a/libavformat/dv.c
+++ b/libavformat/dv.c
@@ -33,6 +33,7 @@
 
 #include 
 #include "avformat.h"
+#include "demux.h"
 #include "internal.h"
 #include "libavcodec/dv_profile.h"
 #include "libavcodec/dv.h"
@@ -46,7 +47,7 @@
 #if CONFIG_DV_DEMUXER
 
 // Must be kept in sync with AVPacket
-struct DVPacket {
+typedef struct DVPacket {
 int64_t  pts;
 uint8_t *data;
 int  size;
@@ -54,7 +55,10 @@ struct DVPacket {
 int  flags;
 int64_t  pos;
 int64_t  duration;
-};
+
+int sample_rate;
+int last_sample_rate;
+} DVPacket;
 
 struct DVDemuxContext {
 const AVDVProfile*  sys;/* Current DV profile. E.g.: 525/60, 625/50 */
@@ -237,7 +241,7 @@ static int dv_extract_audio(const uint8_t *frame, uint8_t 
**ppcm,
 static int dv_extract_audio_info(DVDemuxContext *c, const uint8_t *frame)
 {
 const uint8_t *as_pack;
-int freq, stype, smpls, quant, i, ach;
+int freq, stype, smpls, quant, i, ach, sr;
 
 as_pack = dv_extract_pack(frame, DV_AUDIO_SOURCE);
 if (!as_pack || !c->sys) {/* No audio ? */
@@ -255,6 +259,7 @@ static int dv_extract_audio_info(DVDemuxContext *c, const 
uint8_t *frame)
"Unrecognized audio sample rate index (%d)\n", freq);
 return 0;
 }
+sr = dv_audio_frequency[freq];
 
 if (stype > 3) {
 av_log(c->fctx, AV_LOG_ERROR, "stype %d is invalid\n", stype);
@@ -280,7 +285,10 @@ static int dv_extract_audio_info(DVDemuxContext *c, const 
uint8_t *frame)
 c->ast[i]->codecpar->codec_id   = AV_CODEC_ID_PCM_S16LE;
 c->ast[i]->codecpar->ch_layout  = 
(AVChannelLayout)AV_CHANNEL_LAYOUT_STEREO;
 c->ast[i]->start_time   = 0;
-c->ast[i]->codecpar->bit_rate   = 2 * dv_audio_frequency[freq] * 
16;
+c->ast[i]->codecpar->bit_rate   = 2 * sr * 16;
+
+c->ast[i]->codecpar->sample_rate = sr;
+c->audio_pkt[i].last_sample_rate = sr;
 
 c->audio_pkt[i].size = 0;
 c->audio_pkt[i].data = c->audio_buf[i];
@@ -290,7 +298,8 @@ static int dv_extract_audio_info(DVDemuxContext *c, const 
uint8_t *frame)
 c->audio_pkt[i].duration = 0;
 c->audio_pkt[i].pos  = -1;
 }
-c->ast[i]->codecpar->sample_rate= dv_audio_frequency[freq];
+
+c->audio_pkt[i].sample_rate = sr;
 }
 c->ach = ach;
 
@@ -380,16 +389,26 @@ int avpriv_dv_get_packet(DVDemuxContext *c, AVPacket *pkt)
 
 for (i = 0; i < c->ach; i++) {
 if (c->ast[i] && c->audio_pkt[i].size) {
-pkt->size = c->audio_pkt[i].size;
-pkt->data = c->audio_pkt[i].data;
-pkt->stream_index = c->audio_pkt[i].stream_index;
-pkt->flags= c->audio_pkt[i].flags;
-pkt->pts  = c->audio_pkt[i].pts;
-pkt->duration = c->audio_pkt[i].duration;
-pkt->pos  = c->audio_pkt[i].pos;
-
-c->audio_pkt[i].size = 0;
-size = pkt->size;
+DVPacket *dpkt = >audio_pkt[i];
+
+pkt->size = dpkt->size;
+pkt->data = dpkt->data;
+pkt->stream_index = dpkt->stream_index;
+pkt->flags= dpkt->flags;
+pkt->pts  = dpkt->pts;
+pkt->duration = dpkt->duration;
+pkt->pos  = dpkt->pos;
+
+dpkt->size = 0;
+size   = pkt->size;
+
+if (dpkt->last_sample_rate != dpkt->sample_rate) {
+int ret = ff_add_param_change(pkt, 0, 0, dpkt->sample_rate, 0, 
0);
+if (ret < 0)
+return ret;
+dpkt->last_sample_rate = dpkt->sample_rate;
+}
+
 break;
 }
 }
-- 
2.35.1

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