| Message ID | 20220703181525.6488-1-pal@sandflow.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v1] avformat/imfdec: preserve stream information | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
pal@sandflow.com: > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > As discussed at https://trac.ffmpeg.org/ticket/9818, the IMF demuxer does not > currently preserve stream information such as language in the case of audio > streams. This patch is modeled on copy_stream_props() at avformat/concatdec.c. > > --- > libavformat/imfdec.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > index 71dfb26958..7aa66a06bf 100644 > --- a/libavformat/imfdec.c > +++ b/libavformat/imfdec.c > @@ -580,11 +580,16 @@ static int set_context_streams_from_tracks(AVFormatContext *s) > return AVERROR(ENOMEM); > } > asset_stream->id = i; > + asset_stream->r_frame_rate = first_resource_stream->r_frame_rate; > + asset_stream->avg_frame_rate = first_resource_stream->avg_frame_rate; > + asset_stream->sample_aspect_ratio = first_resource_stream->sample_aspect_ratio; > ret = avcodec_parameters_copy(asset_stream->codecpar, first_resource_stream->codecpar); > if (ret < 0) { > av_log(s, AV_LOG_ERROR, "Could not copy stream parameters\n"); > return ret; > } > + av_dict_copy(&asset_stream->metadata, first_resource_stream->metadata, 0); > + ff_stream_side_data_copy(asset_stream, first_resource_stream); > avpriv_set_pts_info(asset_stream, > first_resource_stream->pts_wrap_bits, > first_resource_stream->time_base.num, Seems to me like one should use ff_stream_encode_params_copy here. Of course, it would have to be renamed and moved if used in a demuxer. - Andreas
On Sun, Jul 3, 2022 at 11:28 AM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > pal@sandflow.com: > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > As discussed at https://trac.ffmpeg.org/ticket/9818, the IMF demuxer does not > > currently preserve stream information such as language in the case of audio > > streams. This patch is modeled on copy_stream_props() at avformat/concatdec.c. > > > > --- > > libavformat/imfdec.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > > index 71dfb26958..7aa66a06bf 100644 > > --- a/libavformat/imfdec.c > > +++ b/libavformat/imfdec.c > > @@ -580,11 +580,16 @@ static int set_context_streams_from_tracks(AVFormatContext *s) > > return AVERROR(ENOMEM); > > } > > asset_stream->id = i; > > + asset_stream->r_frame_rate = first_resource_stream->r_frame_rate; > > + asset_stream->avg_frame_rate = first_resource_stream->avg_frame_rate; > > + asset_stream->sample_aspect_ratio = first_resource_stream->sample_aspect_ratio; > > ret = avcodec_parameters_copy(asset_stream->codecpar, first_resource_stream->codecpar); > > if (ret < 0) { > > av_log(s, AV_LOG_ERROR, "Could not copy stream parameters\n"); > > return ret; > > } > > + av_dict_copy(&asset_stream->metadata, first_resource_stream->metadata, 0); > > + ff_stream_side_data_copy(asset_stream, first_resource_stream); > > avpriv_set_pts_info(asset_stream, > > first_resource_stream->pts_wrap_bits, > > first_resource_stream->time_base.num, > > Seems to me like one should use ff_stream_encode_params_copy here. Of > course, it would have to be renamed and moved if used in a demuxer. Would copy_stream_props() in concatdec.c need to be refactored as well? Note that, in the case of avformat/imfdec.c, AVStream::id is not copied across, so ff_stream_encode_params_copy() would need to be followed by asset_stream->id = i; > > - Andreas > _______________________________________________ > 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".
Pierre-Anthony Lemieux: > On Sun, Jul 3, 2022 at 11:28 AM Andreas Rheinhardt > <andreas.rheinhardt@outlook.com> wrote: >> >> pal@sandflow.com: >>> From: Pierre-Anthony Lemieux <pal@palemieux.com> >>> >>> As discussed at https://trac.ffmpeg.org/ticket/9818, the IMF demuxer does not >>> currently preserve stream information such as language in the case of audio >>> streams. This patch is modeled on copy_stream_props() at avformat/concatdec.c. >>> >>> --- >>> libavformat/imfdec.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c >>> index 71dfb26958..7aa66a06bf 100644 >>> --- a/libavformat/imfdec.c >>> +++ b/libavformat/imfdec.c >>> @@ -580,11 +580,16 @@ static int set_context_streams_from_tracks(AVFormatContext *s) >>> return AVERROR(ENOMEM); >>> } >>> asset_stream->id = i; >>> + asset_stream->r_frame_rate = first_resource_stream->r_frame_rate; >>> + asset_stream->avg_frame_rate = first_resource_stream->avg_frame_rate; >>> + asset_stream->sample_aspect_ratio = first_resource_stream->sample_aspect_ratio; >>> ret = avcodec_parameters_copy(asset_stream->codecpar, first_resource_stream->codecpar); >>> if (ret < 0) { >>> av_log(s, AV_LOG_ERROR, "Could not copy stream parameters\n"); >>> return ret; >>> } >>> + av_dict_copy(&asset_stream->metadata, first_resource_stream->metadata, 0); >>> + ff_stream_side_data_copy(asset_stream, first_resource_stream); >>> avpriv_set_pts_info(asset_stream, >>> first_resource_stream->pts_wrap_bits, >>> first_resource_stream->time_base.num, >> >> Seems to me like one should use ff_stream_encode_params_copy here. Of >> course, it would have to be renamed and moved if used in a demuxer. > > Would copy_stream_props() in concatdec.c need to be refactored as well? > I often wondered about this. The problem with copy_stream_props is that it is not only called during read_header, but lateron as well, but e.g. the documentation of AVStream.side_data says that it is populated when the stream is created and not later. This issue does of course not exist in your case. > Note that, in the case of avformat/imfdec.c, AVStream::id is not > copied across, so ff_stream_encode_params_copy() would need to be > followed by asset_stream->id = i; > Yeah, I know. - Andreas
On Sun, Jul 3, 2022 at 12:15 PM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > Pierre-Anthony Lemieux: > > On Sun, Jul 3, 2022 at 11:28 AM Andreas Rheinhardt > > <andreas.rheinhardt@outlook.com> wrote: > >> > >> pal@sandflow.com: > >>> From: Pierre-Anthony Lemieux <pal@palemieux.com> > >>> > >>> As discussed at https://trac.ffmpeg.org/ticket/9818, the IMF demuxer does not > >>> currently preserve stream information such as language in the case of audio > >>> streams. This patch is modeled on copy_stream_props() at avformat/concatdec.c. > >>> > >>> --- > >>> libavformat/imfdec.c | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> > >>> diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > >>> index 71dfb26958..7aa66a06bf 100644 > >>> --- a/libavformat/imfdec.c > >>> +++ b/libavformat/imfdec.c > >>> @@ -580,11 +580,16 @@ static int set_context_streams_from_tracks(AVFormatContext *s) > >>> return AVERROR(ENOMEM); > >>> } > >>> asset_stream->id = i; > >>> + asset_stream->r_frame_rate = first_resource_stream->r_frame_rate; > >>> + asset_stream->avg_frame_rate = first_resource_stream->avg_frame_rate; > >>> + asset_stream->sample_aspect_ratio = first_resource_stream->sample_aspect_ratio; > >>> ret = avcodec_parameters_copy(asset_stream->codecpar, first_resource_stream->codecpar); > >>> if (ret < 0) { > >>> av_log(s, AV_LOG_ERROR, "Could not copy stream parameters\n"); > >>> return ret; > >>> } > >>> + av_dict_copy(&asset_stream->metadata, first_resource_stream->metadata, 0); > >>> + ff_stream_side_data_copy(asset_stream, first_resource_stream); > >>> avpriv_set_pts_info(asset_stream, > >>> first_resource_stream->pts_wrap_bits, > >>> first_resource_stream->time_base.num, > >> > >> Seems to me like one should use ff_stream_encode_params_copy here. Of > >> course, it would have to be renamed and moved if used in a demuxer. > > > > Would copy_stream_props() in concatdec.c need to be refactored as well? > > > > I often wondered about this. The problem with copy_stream_props is that > it is not only called during read_header, but lateron as well, but e.g. > the documentation of AVStream.side_data says that it is populated when > the stream is created and not later. > This issue does of course not exist in your case. ff_stream_encode_params_copy() does not seem to set pts_wrap_bits, i.e. it does not call avpriv_set_pts_info(). > > > Note that, in the case of avformat/imfdec.c, AVStream::id is not > > copied across, so ff_stream_encode_params_copy() would need to be > > followed by asset_stream->id = i; > > > > Yeah, I know. > > - Andreas > _______________________________________________ > 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".
Pierre-Anthony Lemieux: > On Sun, Jul 3, 2022 at 12:15 PM Andreas Rheinhardt > <andreas.rheinhardt@outlook.com> wrote: >> >> Pierre-Anthony Lemieux: >>> On Sun, Jul 3, 2022 at 11:28 AM Andreas Rheinhardt >>> <andreas.rheinhardt@outlook.com> wrote: >>>> >>>> pal@sandflow.com: >>>>> From: Pierre-Anthony Lemieux <pal@palemieux.com> >>>>> >>>>> As discussed at https://trac.ffmpeg.org/ticket/9818, the IMF demuxer does not >>>>> currently preserve stream information such as language in the case of audio >>>>> streams. This patch is modeled on copy_stream_props() at avformat/concatdec.c. >>>>> >>>>> --- >>>>> libavformat/imfdec.c | 5 +++++ >>>>> 1 file changed, 5 insertions(+) >>>>> >>>>> diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c >>>>> index 71dfb26958..7aa66a06bf 100644 >>>>> --- a/libavformat/imfdec.c >>>>> +++ b/libavformat/imfdec.c >>>>> @@ -580,11 +580,16 @@ static int set_context_streams_from_tracks(AVFormatContext *s) >>>>> return AVERROR(ENOMEM); >>>>> } >>>>> asset_stream->id = i; >>>>> + asset_stream->r_frame_rate = first_resource_stream->r_frame_rate; >>>>> + asset_stream->avg_frame_rate = first_resource_stream->avg_frame_rate; >>>>> + asset_stream->sample_aspect_ratio = first_resource_stream->sample_aspect_ratio; >>>>> ret = avcodec_parameters_copy(asset_stream->codecpar, first_resource_stream->codecpar); >>>>> if (ret < 0) { >>>>> av_log(s, AV_LOG_ERROR, "Could not copy stream parameters\n"); >>>>> return ret; >>>>> } >>>>> + av_dict_copy(&asset_stream->metadata, first_resource_stream->metadata, 0); >>>>> + ff_stream_side_data_copy(asset_stream, first_resource_stream); >>>>> avpriv_set_pts_info(asset_stream, >>>>> first_resource_stream->pts_wrap_bits, >>>>> first_resource_stream->time_base.num, >>>> >>>> Seems to me like one should use ff_stream_encode_params_copy here. Of >>>> course, it would have to be renamed and moved if used in a demuxer. >>> >>> Would copy_stream_props() in concatdec.c need to be refactored as well? >>> >> >> I often wondered about this. The problem with copy_stream_props is that >> it is not only called during read_header, but lateron as well, but e.g. >> the documentation of AVStream.side_data says that it is populated when >> the stream is created and not later. >> This issue does of course not exist in your case. > > ff_stream_encode_params_copy() does not seem to set pts_wrap_bits, > i.e. it does not call avpriv_set_pts_info(). > The reason for this is that pts_wrap_bits is irrelevant for muxing. >> >>> Note that, in the case of avformat/imfdec.c, AVStream::id is not >>> copied across, so ff_stream_encode_params_copy() would need to be >>> followed by asset_stream->id = i; >>> >> >> Yeah, I know. >> >> - Andreas >> _______________________________________________ >> 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". > _______________________________________________ > 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".
On Sun, Jul 3, 2022 at 2:13 PM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > Pierre-Anthony Lemieux: > > On Sun, Jul 3, 2022 at 12:15 PM Andreas Rheinhardt > > <andreas.rheinhardt@outlook.com> wrote: > >> > >> Pierre-Anthony Lemieux: > >>> On Sun, Jul 3, 2022 at 11:28 AM Andreas Rheinhardt > >>> <andreas.rheinhardt@outlook.com> wrote: > >>>> > >>>> pal@sandflow.com: > >>>>> From: Pierre-Anthony Lemieux <pal@palemieux.com> > >>>>> > >>>>> As discussed at https://trac.ffmpeg.org/ticket/9818, the IMF demuxer does not > >>>>> currently preserve stream information such as language in the case of audio > >>>>> streams. This patch is modeled on copy_stream_props() at avformat/concatdec.c. > >>>>> > >>>>> --- > >>>>> libavformat/imfdec.c | 5 +++++ > >>>>> 1 file changed, 5 insertions(+) > >>>>> > >>>>> diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > >>>>> index 71dfb26958..7aa66a06bf 100644 > >>>>> --- a/libavformat/imfdec.c > >>>>> +++ b/libavformat/imfdec.c > >>>>> @@ -580,11 +580,16 @@ static int set_context_streams_from_tracks(AVFormatContext *s) > >>>>> return AVERROR(ENOMEM); > >>>>> } > >>>>> asset_stream->id = i; > >>>>> + asset_stream->r_frame_rate = first_resource_stream->r_frame_rate; > >>>>> + asset_stream->avg_frame_rate = first_resource_stream->avg_frame_rate; > >>>>> + asset_stream->sample_aspect_ratio = first_resource_stream->sample_aspect_ratio; > >>>>> ret = avcodec_parameters_copy(asset_stream->codecpar, first_resource_stream->codecpar); > >>>>> if (ret < 0) { > >>>>> av_log(s, AV_LOG_ERROR, "Could not copy stream parameters\n"); > >>>>> return ret; > >>>>> } > >>>>> + av_dict_copy(&asset_stream->metadata, first_resource_stream->metadata, 0); > >>>>> + ff_stream_side_data_copy(asset_stream, first_resource_stream); > >>>>> avpriv_set_pts_info(asset_stream, > >>>>> first_resource_stream->pts_wrap_bits, > >>>>> first_resource_stream->time_base.num, > >>>> > >>>> Seems to me like one should use ff_stream_encode_params_copy here. Of > >>>> course, it would have to be renamed and moved if used in a demuxer. > >>> > >>> Would copy_stream_props() in concatdec.c need to be refactored as well? > >>> > >> > >> I often wondered about this. The problem with copy_stream_props is that > >> it is not only called during read_header, but lateron as well, but e.g. > >> the documentation of AVStream.side_data says that it is populated when > >> the stream is created and not later. > >> This issue does of course not exist in your case. > > > > ff_stream_encode_params_copy() does not seem to set pts_wrap_bits, > > i.e. it does not call avpriv_set_pts_info(). > > > > The reason for this is that pts_wrap_bits is irrelevant for muxing. Do you have in mind: (a) a single av_stream_params_copy(), which copies all parameters (b) two separate functions av_stream_demux_params_copy() and av_stream_mux_params_copy(), which copies only relevant parameters? (c) something else? In the case of (b), should both be defined now, or only av_stream_demux_params_copy() be implemented now since avformat/imfdec would only use av_stream_demux_params_copy()? I have not seen anyone else chime in. > > >> > >>> Note that, in the case of avformat/imfdec.c, AVStream::id is not > >>> copied across, so ff_stream_encode_params_copy() would need to be > >>> followed by asset_stream->id = i; > >>> > >> > >> Yeah, I know. > >> > >> - Andreas > >> _______________________________________________ > >> 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". > > _______________________________________________ > > 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". > > _______________________________________________ > 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".
diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c index 71dfb26958..7aa66a06bf 100644 --- a/libavformat/imfdec.c +++ b/libavformat/imfdec.c @@ -580,11 +580,16 @@ static int set_context_streams_from_tracks(AVFormatContext *s) return AVERROR(ENOMEM); } asset_stream->id = i; + asset_stream->r_frame_rate = first_resource_stream->r_frame_rate; + asset_stream->avg_frame_rate = first_resource_stream->avg_frame_rate; + asset_stream->sample_aspect_ratio = first_resource_stream->sample_aspect_ratio; ret = avcodec_parameters_copy(asset_stream->codecpar, first_resource_stream->codecpar); if (ret < 0) { av_log(s, AV_LOG_ERROR, "Could not copy stream parameters\n"); return ret; } + av_dict_copy(&asset_stream->metadata, first_resource_stream->metadata, 0); + ff_stream_side_data_copy(asset_stream, first_resource_stream); avpriv_set_pts_info(asset_stream, first_resource_stream->pts_wrap_bits, first_resource_stream->time_base.num,
From: Pierre-Anthony Lemieux <pal@palemieux.com> As discussed at https://trac.ffmpeg.org/ticket/9818, the IMF demuxer does not currently preserve stream information such as language in the case of audio streams. This patch is modeled on copy_stream_props() at avformat/concatdec.c. --- libavformat/imfdec.c | 5 +++++ 1 file changed, 5 insertions(+)