Re: [FFmpeg-devel] [PATCH] avformat: move internal AVStream related functions and structs to a separate header

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

James Almer:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavformat/Makefile   |   1 +
>  libavformat/avformat.c | 214 --------------------------
>  libavformat/demux.h    |  29 ----
>  libavformat/internal.h | 291 +----------------------------------
>  libavformat/stream.c   | 243 +++++++++++++++++++++++++++++
>  libavformat/stream.h   | 338 +++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 583 insertions(+), 533 deletions(-)
>  create mode 100644 libavformat/stream.c
>  create mode 100644 libavformat/stream.h
> 

The diff says that this patch does something else than what it claims to
do: It also moves code and not only adds a new header.

> diff --git a/libavformat/demux.h b/libavformat/demux.h
> index 1f57e062f6..0682147776 100644
> --- a/libavformat/demux.h
> +++ b/libavformat/demux.h
> @@ -26,35 +26,6 @@
>  #include "libavcodec/packet.h"
>  #include "avformat.h"
>  
> -#define MAX_STD_TIMEBASES (30*12+30+3+6)
> -typedef struct FFStreamInfo {
> -    int64_t last_dts;
> -    int64_t duration_gcd;
> -    int duration_count;
> -    int64_t rfps_duration_sum;
> -    double (*duration_error)[2][MAX_STD_TIMEBASES];
> -    int64_t codec_info_duration;
> -    int64_t codec_info_duration_fields;
> -    int frame_delay_evidence;
> -
> -    /**
> -     * 0  -> decoder has not been searched for yet.
> -     * >0 -> decoder found
> -     * <0 -> decoder with codec_id == -found_decoder has not been found
> -     */
> -    int found_decoder;
> -
> -    int64_t last_duration;
> -
> -    /**
> -     * Those are used for average framerate estimation.
> -     */
> -    int64_t fps_first_dts;
> -    int     fps_first_dts_idx;
> -    int64_t fps_last_dts;
> -    int     fps_last_dts_idx;
> -} FFStreamInfo;
> -

Why are you moving this to stream.h (which makes it visible to muxers
who have absolutely no business knowing about it)?

>  /**
>   * Returned by demuxers to indicate that data was consumed but discarded
>   * (ignored streams or junk data). The framework will re-call the demuxer.
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 7702986c9c..9996b7a770 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -26,6 +26,7 @@
>  #include "libavcodec/packet_internal.h"
>  
>  #include "avformat.h"
> +#include "stream.h"

What's the point of this (apart from making this patch simpler)?

And what is the point of this patch? When one has a header that includes
stuff that is unused by several files that include said header, moving
said stuff (or its complement) into a new header allows to reduce the
amount of stuff actually included. Yet you are not doing this here and
the FFStreamInfo changes go in the opposite direction.

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