[FFmpeg-devel,12/18] lavf/dv: make returning the video packet optional

The mov demuxer only returns DV audio, video packets are discarded.

It first reads the data to be parsed into a packet. Then both this
packet and the pointer to its data are passed together to
avpriv_dv_produce_packet(), which parses the data and partially
overwrites the packet. This is confusing and potentially dangerous, so
just pass NULL and avoid pointless packet modification.
---
 libavformat/dv.c  | 19 +++++++++++--------
 libavformat/mov.c |  2 +-
 2 files changed, 12 insertions(+), 9 deletions(-)

Anton Khirnov:
> The mov demuxer only returns DV audio, video packets are discarded.
> 
> It first reads the data to be parsed into a packet. Then both this
> packet and the pointer to its data are passed together to
> avpriv_dv_produce_packet(), which parses the data and partially
> overwrites the packet. This is confusing and potentially dangerous, so
> just pass NULL and avoid pointless packet modification.
> ---
>  libavformat/dv.c  | 19 +++++++++++--------
>  libavformat/mov.c |  2 +-
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/dv.c b/libavformat/dv.c
> index 303cecf9bb..f88fe62349 100644
> --- a/libavformat/dv.c
> +++ b/libavformat/dv.c
> @@ -430,14 +430,17 @@ int avpriv_dv_produce_packet(DVDemuxContext *c, AVPacket *pkt,
>          }
>      }
>  
> -    /* Now it's time to return video packet */
> -    size = dv_extract_video_info(c, buf);
> -    pkt->data         = buf;
> -    pkt->pos          = pos;
> -    pkt->size         = size;
> -    pkt->flags       |= AV_PKT_FLAG_KEY;
> -    pkt->stream_index = c->vst->index;
> -    pkt->pts          = c->frames;
> +    /* return the video packet, if the caller wants it */
> +    if (pkt) {
> +        size = dv_extract_video_info(c, buf);
> +
> +        pkt->data         = buf;
> +        pkt->pos          = pos;
> +        pkt->size         = size;
> +        pkt->flags       |= AV_PKT_FLAG_KEY;
> +        pkt->stream_index = c->vst->index;
> +        pkt->pts          = c->frames;
> +    }
>  
>      c->frames++;
>  
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 1d8c5b2904..a2b429e52f 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -8768,7 +8768,7 @@ static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
>          }
>  #if CONFIG_DV_DEMUXER
>          if (mov->dv_demux && sc->dv_audio_container) {
> -            ret = avpriv_dv_produce_packet(mov->dv_demux, pkt, pkt->data, pkt->size, pkt->pos);
> +            ret = avpriv_dv_produce_packet(mov->dv_demux, NULL, pkt->data, pkt->size, pkt->pos);
>              av_packet_unref(pkt);
>              if (ret < 0)
>                  return ret;

1. LGTM.
2. The way mov handles dv audio is very broken:
a) I don't see anything that guarantees that the
samplerate of the inner demuxer and the sample rate reported to the user
(presumably taken from mov structures) coincide.
b) dv_fctx and dv_demux are part of MOVContext, not MOVStreamContext. If
there were multiple MOVStreams with dv-audio, the older dv_fctx and
dv_demux would leak. Do you see anything that guarantees that they will
 only be at most one MOVStream for dv-audio?
c) The former would be easily fixable by moving the fields to
MOVStreamContext. But there is actually another bug: The underlying dv
stream can contain multiple audio streams in this one MOVStream and only
the first of these is forwarded.
https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket4671/dir1.tar.bz2
contains a sample where this actually occurs: The DV stream has two
stereo tracks, only the first of which is forwarded (use -enable_drefs 1
to be able to play it; the non-forwarded seems to be silent in this
case). Are these actually independent audios or are these stereo pairs
part of a single streams with more than two channels?

- Andreas

Quoting Andreas Rheinhardt (2022-08-24 18:03:05)
> Anton Khirnov:
> > The mov demuxer only returns DV audio, video packets are discarded.
> > 
> > It first reads the data to be parsed into a packet. Then both this
> > packet and the pointer to its data are passed together to
> > avpriv_dv_produce_packet(), which parses the data and partially
> > overwrites the packet. This is confusing and potentially dangerous, so
> > just pass NULL and avoid pointless packet modification.
> > ---
> >  libavformat/dv.c  | 19 +++++++++++--------
> >  libavformat/mov.c |  2 +-
> >  2 files changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/libavformat/dv.c b/libavformat/dv.c
> > index 303cecf9bb..f88fe62349 100644
> > --- a/libavformat/dv.c
> > +++ b/libavformat/dv.c
> > @@ -430,14 +430,17 @@ int avpriv_dv_produce_packet(DVDemuxContext *c, AVPacket *pkt,
> >          }
> >      }
> >  
> > -    /* Now it's time to return video packet */
> > -    size = dv_extract_video_info(c, buf);
> > -    pkt->data         = buf;
> > -    pkt->pos          = pos;
> > -    pkt->size         = size;
> > -    pkt->flags       |= AV_PKT_FLAG_KEY;
> > -    pkt->stream_index = c->vst->index;
> > -    pkt->pts          = c->frames;
> > +    /* return the video packet, if the caller wants it */
> > +    if (pkt) {
> > +        size = dv_extract_video_info(c, buf);
> > +
> > +        pkt->data         = buf;
> > +        pkt->pos          = pos;
> > +        pkt->size         = size;
> > +        pkt->flags       |= AV_PKT_FLAG_KEY;
> > +        pkt->stream_index = c->vst->index;
> > +        pkt->pts          = c->frames;
> > +    }
> >  
> >      c->frames++;
> >  
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 1d8c5b2904..a2b429e52f 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -8768,7 +8768,7 @@ static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
> >          }
> >  #if CONFIG_DV_DEMUXER
> >          if (mov->dv_demux && sc->dv_audio_container) {
> > -            ret = avpriv_dv_produce_packet(mov->dv_demux, pkt, pkt->data, pkt->size, pkt->pos);
> > +            ret = avpriv_dv_produce_packet(mov->dv_demux, NULL, pkt->data, pkt->size, pkt->pos);
> >              av_packet_unref(pkt);
> >              if (ret < 0)
> >                  return ret;
> 
> 1. LGTM.
> 2. The way mov handles dv audio is very broken:
> a) I don't see anything that guarantees that the
> samplerate of the inner demuxer and the sample rate reported to the user
> (presumably taken from mov structures) coincide.
> b) dv_fctx and dv_demux are part of MOVContext, not MOVStreamContext. If
> there were multiple MOVStreams with dv-audio, the older dv_fctx and
> dv_demux would leak. Do you see anything that guarantees that they will
>  only be at most one MOVStream for dv-audio?

Not that I can see. I'd say the best thing to do here is fail when a
dvaudio stream already exists - trying to support it without having any
valid samples seems pointless. I'll send a patch.

> c) The former would be easily fixable by moving the fields to
> MOVStreamContext. But there is actually another bug: The underlying dv
> stream can contain multiple audio streams in this one MOVStream and only
> the first of these is forwarded.
> https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket4671/dir1.tar.bz2
> contains a sample where this actually occurs: The DV stream has two
> stereo tracks, only the first of which is forwarded (use -enable_drefs 1
> to be able to play it; the non-forwarded seems to be silent in this
> case). Are these actually independent audios or are these stereo pairs
> part of a single streams with more than two channels?

Cannot answer this. I would ignore this until and unless someone
actually comes complaining about this.