| Message ID | 20240607130135.9088-22-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/39] lavc/hevcdec: do not free SliceHeader arrays in pic_arrays_free() | expand |
Le ven. 7 juin 2024, 15:07, Anton Khirnov <anton@khirnov.net> a écrit : > if (pps->tiles_enabled_flag && > pps->tile_id[ctb_addr_ts] != pps->tile_id[ctb_addr_ts - 1]) { > int ret; > - if (s->threads_number == 1) > + if (!is_wpp) > ret = cabac_reinit(lc); > else { > ret = ff_init_cabac_decoder(&lc->cc, data, size); > I have 2 things to say about this. First is that IIRC, tiles and WPP are mutually exclusive, so I would have expected your change to introduce dead code. It might have been changed for a later profile, but I think ffmpeg's decoder doesn't support this or these profiles. Anyway, I wonder what the intent of that code was. Which leads me to the second. FFmpeg decoder does not support tile threading (the one it's based off does/did) so maybe this code never mattered for FFmpeg. Hesitant LGTM, as I find this fishy and wondering if I'm missing something >
Quoting Christophe Gisquet (2024-06-08 09:02:04) > Le ven. 7 juin 2024, 15:07, Anton Khirnov <anton@khirnov.net> a écrit : > > > if (pps->tiles_enabled_flag && > > pps->tile_id[ctb_addr_ts] != pps->tile_id[ctb_addr_ts - 1]) { > > int ret; > > - if (s->threads_number == 1) > > + if (!is_wpp) > > ret = cabac_reinit(lc); > > else { > > ret = ff_init_cabac_decoder(&lc->cc, data, size); > > > > I have 2 things to say about this. First is that IIRC, tiles and WPP are > mutually exclusive, so I would have expected your change to introduce dead > code. It might have been changed for a later profile, but I think ffmpeg's > decoder doesn't support this or these profiles. Anyway, I wonder what the > intent of that code was. > > Which leads me to the second. FFmpeg decoder does not support tile > threading (the one it's based off does/did) so maybe this code never > mattered for FFmpeg. > > Hesitant LGTM, as I find this fishy and wondering if I'm missing something You are correct, tiles and WPP are mutually exclusive and we do not support tile threading. However, I'm not _introducing_ dead code, as the code in question is already there and already dead. I've considered removing it, but decided against it 1) to keep it consistent with the similar branch below. 2) since I don't really understand how it works
diff --git a/libavcodec/hevc/cabac.c b/libavcodec/hevc/cabac.c index 8708efc248..39ca7c0135 100644 --- a/libavcodec/hevc/cabac.c +++ b/libavcodec/hevc/cabac.c @@ -452,7 +452,8 @@ static void cabac_init_state(HEVCLocalContext *lc, const HEVCContext *s) } int ff_hevc_cabac_init(HEVCLocalContext *lc, const HEVCPPS *pps, - int ctb_addr_ts, const uint8_t *data, size_t size) + int ctb_addr_ts, const uint8_t *data, size_t size, + int is_wpp) { const HEVCContext *const s = lc->parent; const HEVCSPS *const sps = pps->sps; @@ -479,7 +480,7 @@ int ff_hevc_cabac_init(HEVCLocalContext *lc, const HEVCPPS *pps, if (pps->tiles_enabled_flag && pps->tile_id[ctb_addr_ts] != pps->tile_id[ctb_addr_ts - 1]) { int ret; - if (s->threads_number == 1) + if (!is_wpp) ret = cabac_reinit(lc); else { ret = ff_init_cabac_decoder(&lc->cc, data, size); @@ -492,7 +493,7 @@ int ff_hevc_cabac_init(HEVCLocalContext *lc, const HEVCPPS *pps, if (ctb_addr_ts % sps->ctb_width == 0) { int ret; get_cabac_terminate(&lc->cc); - if (s->threads_number == 1) + if (!is_wpp) ret = cabac_reinit(lc); else { ret = ff_init_cabac_decoder(&lc->cc, data, size); diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c index a57fa4e539..e3773a6147 100644 --- a/libavcodec/hevc/hevcdec.c +++ b/libavcodec/hevc/hevcdec.c @@ -2599,7 +2599,7 @@ static int hls_decode_entry(HEVCContext *s, GetBitContext *gb) y_ctb = (ctb_addr_rs / ((sps->width + ctb_size - 1) >> sps->log2_ctb_size)) << sps->log2_ctb_size; hls_decode_neighbour(lc, pps, sps, x_ctb, y_ctb, ctb_addr_ts); - ret = ff_hevc_cabac_init(lc, pps, ctb_addr_ts, slice_data, slice_size); + ret = ff_hevc_cabac_init(lc, pps, ctb_addr_ts, slice_data, slice_size, 0); if (ret < 0) { s->tab_slice_address[ctb_addr_rs] = -1; return ret; @@ -2669,7 +2669,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, return 0; } - ret = ff_hevc_cabac_init(lc, pps, ctb_addr_ts, data, data_size); + ret = ff_hevc_cabac_init(lc, pps, ctb_addr_ts, data, data_size, 1); if (ret < 0) goto error; hls_sao_param(lc, pps, sps, diff --git a/libavcodec/hevc/hevcdec.h b/libavcodec/hevc/hevcdec.h index 04eacca76d..22367602aa 100644 --- a/libavcodec/hevc/hevcdec.h +++ b/libavcodec/hevc/hevcdec.h @@ -579,7 +579,8 @@ int ff_hevc_slice_rpl(HEVCContext *s); void ff_hevc_save_states(HEVCLocalContext *lc, const HEVCPPS *pps, int ctb_addr_ts); int ff_hevc_cabac_init(HEVCLocalContext *lc, const HEVCPPS *pps, - int ctb_addr_ts, const uint8_t *data, size_t size); + int ctb_addr_ts, const uint8_t *data, size_t size, + int is_wpp); int ff_hevc_sao_merge_flag_decode(HEVCLocalContext *lc); int ff_hevc_sao_type_idx_decode(HEVCLocalContext *lc); int ff_hevc_sao_band_position_decode(HEVCLocalContext *lc);