Re: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_parse: change prefix to avpriv for usage in avformat mxf muxer
On Wed, Apr 10, 2019 at 12:44 AM Mark Thompson wrote:
>
> On 09/04/2019 23:14, Baptiste Coudurier wrote:
> > ---
> > libavcodec/h264_parse.c | 2 +-
> > libavcodec/h264_parser.c | 2 +-
> > libavcodec/h264_ps.c | 4 ++--
> > libavcodec/h264_ps.h | 4 ++--
> > libavcodec/h264dec.c | 6 +++---
> > 5 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
> > index 17bfa780ce..980b1e189d 100644
> > --- a/libavcodec/h264_ps.c
> > +++ b/libavcodec/h264_ps.c
> > @@ -330,8 +330,8 @@ void ff_h264_ps_uninit(H264ParamSets *ps)
> > ps->sps = NULL;
> > }
> >
> > -int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext
> > *avctx,
> > - H264ParamSets *ps, int
> > ignore_truncation)
> > +int avpriv_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext
> > *avctx,
> > + H264ParamSets *ps, int
> > ignore_truncation)
> > {
> > AVBufferRef *sps_buf;
> > int profile_idc, level_idc, constraint_set_flags = 0;
> > diff --git a/libavcodec/h264_ps.h b/libavcodec/h264_ps.h
> > index e967b9cbcf..d422ce122e 100644
> > --- a/libavcodec/h264_ps.h
> > +++ b/libavcodec/h264_ps.h
> > @@ -149,8 +149,8 @@ typedef struct H264ParamSets {
> > /**
> > * Decode SPS
> > */
> > -int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext
> > *avctx,
> > - H264ParamSets *ps, int
> > ignore_truncation);
> > +int avpriv_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext
> > *avctx,
> > + H264ParamSets *ps, int
> > ignore_truncation);
>
> Making the H.264 decoder's internal SPS and PPS structures fixed API really
> doesn't feel like a good idea to me, but I admit I don't have a better answer
> to the problem you're facing. Copying everything is also pretty terrible.
> Adding a new API to parse the headers and return the information you want
> might be nicer considering just this change, but extensibility for the
> inevitable subsequent patch which wants some extra piece of information in
> future would be a big pain.
>
> If you're going to go with this, please add namespace prefixes to the
> structures it affects ("SPS" and "PPS", since you're now including them in
> code which isn't explicitly H.264), and also add big warnings to the header
> indicating that the structures are now fixed and can't be modified at all
> except on major bumps. (Though maybe wait for other comments before actually
> making any changes.)
>
I agree that I really don't like exporting these functions. For
example, this function takes an AVCodecContext. Its reasonable to
assume when touching this function right now that its a valid
AVCodecContext with a valid H264Context inside of it. Apparently right
now its only used for logging, so some crazy cast appears to work, but
in the future?
Internal decoder functionality has no place being exported, it puts
too many limits in place for future changes.
Either the API needs to be wrapped in a proper external interface,
leaving our internal interface open to changes, or a simplified
version should be implemented in avformat, like we already did for a
bunch of other video formats as it was required. The SPS isn't that
complex tbh, so maybe implementing a simplified parser for mxf might
be the best option.
- Hendrik
___
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 1/2] avcodec/h264_parse: change prefix to avpriv for usage in avformat mxf muxer
On 09/04/2019 23:14, Baptiste Coudurier wrote:
> ---
> libavcodec/h264_parse.c | 2 +-
> libavcodec/h264_parser.c | 2 +-
> libavcodec/h264_ps.c | 4 ++--
> libavcodec/h264_ps.h | 4 ++--
> libavcodec/h264dec.c | 6 +++---
> 5 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
> index 17bfa780ce..980b1e189d 100644
> --- a/libavcodec/h264_ps.c
> +++ b/libavcodec/h264_ps.c
> @@ -330,8 +330,8 @@ void ff_h264_ps_uninit(H264ParamSets *ps)
> ps->sps = NULL;
> }
>
> -int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext
> *avctx,
> - H264ParamSets *ps, int
> ignore_truncation)
> +int avpriv_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext
> *avctx,
> + H264ParamSets *ps, int
> ignore_truncation)
> {
> AVBufferRef *sps_buf;
> int profile_idc, level_idc, constraint_set_flags = 0;
> diff --git a/libavcodec/h264_ps.h b/libavcodec/h264_ps.h
> index e967b9cbcf..d422ce122e 100644
> --- a/libavcodec/h264_ps.h
> +++ b/libavcodec/h264_ps.h
> @@ -149,8 +149,8 @@ typedef struct H264ParamSets {
> /**
> * Decode SPS
> */
> -int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext
> *avctx,
> - H264ParamSets *ps, int
> ignore_truncation);
> +int avpriv_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext
> *avctx,
> + H264ParamSets *ps, int
> ignore_truncation);
Making the H.264 decoder's internal SPS and PPS structures fixed API really
doesn't feel like a good idea to me, but I admit I don't have a better answer
to the problem you're facing. Copying everything is also pretty terrible.
Adding a new API to parse the headers and return the information you want might
be nicer considering just this change, but extensibility for the inevitable
subsequent patch which wants some extra piece of information in future would be
a big pain.
If you're going to go with this, please add namespace prefixes to the
structures it affects ("SPS" and "PPS", since you're now including them in code
which isn't explicitly H.264), and also add big warnings to the header
indicating that the structures are now fixed and can't be modified at all
except on major bumps. (Though maybe wait for other comments before actually
making any changes.)
- Mark
___
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 1/2] avcodec/h264_parse: change prefix to avpriv for usage in avformat mxf muxer
Hi James, I hope you are doing well > On Apr 3, 2019, at 6:59 AM, James Almer wrote: > > On 4/3/2019 9:25 AM, Baptiste Coudurier wrote: >> Hey Hendrik >> >>> On Apr 3, 2019, at 11:37 AM, Hendrik Leppkes wrote: >>> >>> On Wed, Apr 3, 2019 at 11:22 AM Baptiste Coudurier >>> wrote: --- libavcodec/cbs_h2645.c | 28 ++-- libavcodec/extract_extradata_bsf.c | 4 ++-- libavcodec/h2645_parse.c | 6 +++--- libavcodec/h2645_parse.h | 6 +++--- libavcodec/h264_parse.c| 4 ++-- libavcodec/h264_parser.c | 4 ++-- libavcodec/h264_ps.c | 4 ++-- libavcodec/h264_ps.h | 4 ++-- libavcodec/h264_slice.c| 2 +- libavcodec/h264data.c | 2 +- libavcodec/h264data.h | 3 ++- libavcodec/h264dec.c | 10 +- libavcodec/hevc_parse.c| 2 +- libavcodec/hevc_parser.c | 4 ++-- libavcodec/hevcdec.c | 4 ++-- libavcodec/svq3.c | 2 +- 16 files changed, 45 insertions(+), 44 deletions(-) >>> >>> We prefer not to expose huge modules like this as avpriv, as it makes >>> it part of the ABI and as such impossible to change without >>> deprecation cycles. >>> Preferably, avpriv should be avoided entirely where possible, as it >>> has many of the same limitations as actual public API, and just adds >>> confusion. >> >> Understood. What’s the alternative ? > > One usual solution is adding an avpriv_ wrapper for the internal > function, like avpriv_ac3_parse_header() is done for > ff_ac3_parse_header(), which prevents internal structs from being part > of the ABI. But ff_h2645_packet_split() and > ff_h264_decode_seq_parameter_set() don't allow for that, seeing > H264ParamSets, H2645Packet, H2645RBSP and H2645NAL are complex and tend > to get changes every now and then. The last time was only a few months ago. > > Other than what you're doing in your patch, the only solution i can > think of is duplicating the functionality. It wouldn't be the first, as > you can see in avc.c, hevc.c, av1.c, where bitstream parsing was > implemented in a reduced form exclusively for muxing purposes. All right, I changed the code to avoid using ff_h2645_packet_split() and I found “nal_unit_extract_rbsp” in libavformat :) I don’t think duplicating ff_h264_decode_seq_parameter would be good though so I still changed the prefix to avpriv. Let me know if that works. Thanks! — Baptiste signature.asc Description: Message signed with OpenPGP ___ 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 1/2] avcodec/h264_parse: change prefix to avpriv for usage in avformat mxf muxer
On 4/3/2019 9:25 AM, Baptiste Coudurier wrote: > Hey Hendrik > >> On Apr 3, 2019, at 11:37 AM, Hendrik Leppkes wrote: >> >> On Wed, Apr 3, 2019 at 11:22 AM Baptiste Coudurier >> wrote: >>> >>> --- >>> libavcodec/cbs_h2645.c | 28 ++-- >>> libavcodec/extract_extradata_bsf.c | 4 ++-- >>> libavcodec/h2645_parse.c | 6 +++--- >>> libavcodec/h2645_parse.h | 6 +++--- >>> libavcodec/h264_parse.c| 4 ++-- >>> libavcodec/h264_parser.c | 4 ++-- >>> libavcodec/h264_ps.c | 4 ++-- >>> libavcodec/h264_ps.h | 4 ++-- >>> libavcodec/h264_slice.c| 2 +- >>> libavcodec/h264data.c | 2 +- >>> libavcodec/h264data.h | 3 ++- >>> libavcodec/h264dec.c | 10 +- >>> libavcodec/hevc_parse.c| 2 +- >>> libavcodec/hevc_parser.c | 4 ++-- >>> libavcodec/hevcdec.c | 4 ++-- >>> libavcodec/svq3.c | 2 +- >>> 16 files changed, 45 insertions(+), 44 deletions(-) >>> >> >> We prefer not to expose huge modules like this as avpriv, as it makes >> it part of the ABI and as such impossible to change without >> deprecation cycles. >> Preferably, avpriv should be avoided entirely where possible, as it >> has many of the same limitations as actual public API, and just adds >> confusion. > > Understood. What’s the alternative ? One usual solution is adding an avpriv_ wrapper for the internal function, like avpriv_ac3_parse_header() is done for ff_ac3_parse_header(), which prevents internal structs from being part of the ABI. But ff_h2645_packet_split() and ff_h264_decode_seq_parameter_set() don't allow for that, seeing H264ParamSets, H2645Packet, H2645RBSP and H2645NAL are complex and tend to get changes every now and then. The last time was only a few months ago. Other than what you're doing in your patch, the only solution i can think of is duplicating the functionality. It wouldn't be the first, as you can see in avc.c, hevc.c, av1.c, where bitstream parsing was implemented in a reduced form exclusively for muxing purposes. > > Thanks! > > — > Baptiste > ___ > 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". > ___ 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 1/2] avcodec/h264_parse: change prefix to avpriv for usage in avformat mxf muxer
On Wed, Apr 3, 2019 at 2:28 PM Carl Eugen Hoyos wrote: > > 2019-04-03 11:37 GMT+02:00, Hendrik Leppkes : > > > We prefer not to expose huge modules like this as avpriv, > > as it makes it part of the ABI > > (I am sure there are cases that are difficult to avoid, > we also prefer not to duplicate functions.) > > > and as such impossible to change without > > deprecation cycles. > > That does not sound correct to me. > You are correct, we don't need deprecation cycles. But we do need a major bump to change it, so thats typically still a up to 2 year delay. - Hendrik ___ 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 1/2] avcodec/h264_parse: change prefix to avpriv for usage in avformat mxf muxer
2019-04-03 11:37 GMT+02:00, Hendrik Leppkes : > We prefer not to expose huge modules like this as avpriv, > as it makes it part of the ABI (I am sure there are cases that are difficult to avoid, we also prefer not to duplicate functions.) > and as such impossible to change without > deprecation cycles. That does not sound correct to me. Carl Eugen ___ 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 1/2] avcodec/h264_parse: change prefix to avpriv for usage in avformat mxf muxer
Hey Hendrik > On Apr 3, 2019, at 11:37 AM, Hendrik Leppkes wrote: > > On Wed, Apr 3, 2019 at 11:22 AM Baptiste Coudurier > wrote: >> >> --- >> libavcodec/cbs_h2645.c | 28 ++-- >> libavcodec/extract_extradata_bsf.c | 4 ++-- >> libavcodec/h2645_parse.c | 6 +++--- >> libavcodec/h2645_parse.h | 6 +++--- >> libavcodec/h264_parse.c| 4 ++-- >> libavcodec/h264_parser.c | 4 ++-- >> libavcodec/h264_ps.c | 4 ++-- >> libavcodec/h264_ps.h | 4 ++-- >> libavcodec/h264_slice.c| 2 +- >> libavcodec/h264data.c | 2 +- >> libavcodec/h264data.h | 3 ++- >> libavcodec/h264dec.c | 10 +- >> libavcodec/hevc_parse.c| 2 +- >> libavcodec/hevc_parser.c | 4 ++-- >> libavcodec/hevcdec.c | 4 ++-- >> libavcodec/svq3.c | 2 +- >> 16 files changed, 45 insertions(+), 44 deletions(-) >> > > We prefer not to expose huge modules like this as avpriv, as it makes > it part of the ABI and as such impossible to change without > deprecation cycles. > Preferably, avpriv should be avoided entirely where possible, as it > has many of the same limitations as actual public API, and just adds > confusion. Understood. What’s the alternative ? Thanks! — Baptiste ___ 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 1/2] avcodec/h264_parse: change prefix to avpriv for usage in avformat mxf muxer
On Wed, Apr 3, 2019 at 11:22 AM Baptiste Coudurier wrote: > > --- > libavcodec/cbs_h2645.c | 28 ++-- > libavcodec/extract_extradata_bsf.c | 4 ++-- > libavcodec/h2645_parse.c | 6 +++--- > libavcodec/h2645_parse.h | 6 +++--- > libavcodec/h264_parse.c| 4 ++-- > libavcodec/h264_parser.c | 4 ++-- > libavcodec/h264_ps.c | 4 ++-- > libavcodec/h264_ps.h | 4 ++-- > libavcodec/h264_slice.c| 2 +- > libavcodec/h264data.c | 2 +- > libavcodec/h264data.h | 3 ++- > libavcodec/h264dec.c | 10 +- > libavcodec/hevc_parse.c| 2 +- > libavcodec/hevc_parser.c | 4 ++-- > libavcodec/hevcdec.c | 4 ++-- > libavcodec/svq3.c | 2 +- > 16 files changed, 45 insertions(+), 44 deletions(-) > We prefer not to expose huge modules like this as avpriv, as it makes it part of the ABI and as such impossible to change without deprecation cycles. Preferably, avpriv should be avoided entirely where possible, as it has many of the same limitations as actual public API, and just adds confusion. - Hendrik ___ 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".
