| Message ID | 20190409221403.41981-1-baptiste.coudurier@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
On Wed, Apr 10, 2019 at 12:44 AM Mark Thompson <sw@jkqxz.net> 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
diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c index a075443d17..0f008c29f3 100644 --- a/libavcodec/h264_parse.c +++ b/libavcodec/h264_parse.c @@ -374,7 +374,7 @@ static int decode_extradata_ps(const uint8_t *data, int size, H264ParamSets *ps, H2645NAL *nal = &pkt.nals[i]; switch (nal->type) { case H264_NAL_SPS: - ret = ff_h264_decode_seq_parameter_set(&nal->gb, logctx, ps, 0); + ret = avpriv_h264_decode_seq_parameter_set(&nal->gb, logctx, ps, 0); if (ret < 0) goto fail; break; diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c index 5f9a9c46ef..814584cc82 100644 --- a/libavcodec/h264_parser.c +++ b/libavcodec/h264_parser.c @@ -323,7 +323,7 @@ static inline int parse_nal_units(AVCodecParserContext *s, switch (nal.type) { case H264_NAL_SPS: - ff_h264_decode_seq_parameter_set(&nal.gb, avctx, &p->ps, 0); + avpriv_h264_decode_seq_parameter_set(&nal.gb, avctx, &p->ps, 0); break; case H264_NAL_PPS: ff_h264_decode_picture_parameter_set(&nal.gb, avctx, &p->ps, 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); /** * Decode PPS diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c index 837c3b7538..2e82e118b7 100644 --- a/libavcodec/h264dec.c +++ b/libavcodec/h264dec.c @@ -716,14 +716,14 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size) if (ret < 0) goto end; } - if (ff_h264_decode_seq_parameter_set(&tmp_gb, avctx, &h->ps, 0) >= 0) + if (avpriv_h264_decode_seq_parameter_set(&tmp_gb, avctx, &h->ps, 0) >= 0) break; av_log(h->avctx, AV_LOG_DEBUG, "SPS decoding failure, trying again with the complete NAL\n"); init_get_bits8(&tmp_gb, nal->raw_data + 1, nal->raw_size - 1); - if (ff_h264_decode_seq_parameter_set(&tmp_gb, avctx, &h->ps, 0) >= 0) + if (avpriv_h264_decode_seq_parameter_set(&tmp_gb, avctx, &h->ps, 0) >= 0) break; - ff_h264_decode_seq_parameter_set(&nal->gb, avctx, &h->ps, 1); + avpriv_h264_decode_seq_parameter_set(&nal->gb, avctx, &h->ps, 1); break; } case H264_NAL_PPS: