On 22/01/13 20:30, Jordi Ortiz wrote:
> Thanks for the review
>
> On 21/01/13 22:55, Diego Biurrun wrote:
>
>> Thanks for getting back to this decoder!
>>
>> On Mon, Jan 21, 2013 at 08:25:39PM +0100, Jordi Ortiz wrote:
>>> ---
>>> Use VideoDSPContext.emulated_edge_mc() instead of ff_emultated_edge_mc_8()
>>> Use the ff_get_buffer() wrapper.
>>>
>>> Changelog | 1 +
>>> configure | 1 +
>>> doc/general.texi | 5 +-
>>> libavcodec/Makefile | 3 +
>>> libavcodec/allcodecs.c | 1 +
>>> libavcodec/dirac_arith.c | 115 +++
>>> libavcodec/dirac_arith.h | 166 ++++
>>> libavcodec/diracdec.c | 2014
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>> libavcodec/version.h | 2 +-
>>> 9 files changed, 2305 insertions(+), 3 deletions(-)
>>> create mode 100644 libavcodec/dirac_arith.c
>>> create mode 100644 libavcodec/dirac_arith.h
>>> create mode 100644 libavcodec/diracdec.c
>> What is the status of this decoder now? Does it still produce artifacts?
>> If yes, for which cases?
>
> The status is the same as it was. When sub pixel motion vector precision
> is used the decoded result isn't bit exact with last libschroedinger
> version.
> Also deep-estimation is not working properly. If anyone knows what
> deep-estimation is and wants to point it out it would be very welcome, I
> didn't manage to find any reference in the standard. I know that deep
> estimation uses mv_precision values over zero so the problem in this
> case could be related to the mv_precision.
Let's just document that and add some avpriv_report_missing_feature.
Would be nice having it in some shape this year.
>>> --- /dev/null
>>> +++ b/libavcodec/dirac_arith.h
>>> @@ -0,0 +1,166 @@
>>> +
>>> +#ifndef AVCODEC_DIRAC_ARITH_H
>>> +#define AVCODEC_DIRAC_ARITH_H
>>> +
>>> +#include "bytestream.h"
>>> +#include "get_bits.h"
>> This does not appear to use anything from get_bits.h.
>
> Moved the include to dirac_arith.c. There GetBitContext is used to
> calculate how much data is left.
Maybe can be moved there?
>>> --- /dev/null
>>> +++ b/libavcodec/diracdec.c
>>> @@ -0,0 +1,2014 @@
>>> +
>>> +static void dirac_decode_flush(AVCodecContext *avctx)
>>> +{
>>> + DiracContext *s = avctx->priv_data;
>>> + free_sequence_buffers(s);
>>> + s->seen_sequence_header = 0;
>>> + s->frame_number = -1;
>>> +}
>>> +
>>> +static av_cold int dirac_decode_end(AVCodecContext *avctx)
>>> +{
>>> + dirac_decode_flush(avctx);
>>> + return 0;
>>> +}
>> Why the indirection?
>
> Might it be because AVCodec.close and AVCodec.flush have different
> return types?
And you are absolutely right.
>>> +/* [DIRAC_STD] 11.2 Picture prediction data. picture_prediction()
>>> + * Unpack the motion compensation parameters. */
>>> +static int dirac_unpack_prediction_parameters(DiracContext *s)
>>> +{
>>> + if (FFMAX(s->plane[0].xblen, s->plane[0].yblen) > DIRAC_MAX_BLOCKSIZE)
>>> {
>>> + av_log(s->avctx, AV_LOG_ERROR, "Unsupported large block size\n");
>>> + return AVERROR_PATCHWELCOME;
>> Is this a candidate for av_log_missing_feature or av_log_patch_welcome?
>
> av_log_missing_feature sounds better.
> Should I change all the av_log() calls to av_log_... calls?
If you do, please use the avpriv
>
>>> +/* [DIRAC_STD] 11.1.1 Picture Header. picture_header() */
>>> +static int dirac_decode_picture_header(DiracContext *s)
>>> +{
>>> + int retire, picnum;
>>> + int i, j, refnum, refdist, ret, distance;
>>> + GetBitContext *gb = &s->gb;
>>> +
>>> + /* [DIRAC_STD] 11.1.1 Picture Header. picture_header() PICTURE_NUM */
>>> + picnum =
>>> + s->current_picture->avframe.display_picture_number =
>>> + get_bits_long(gb, 32);
>>> + picnum = s->current_picture->avframe.display_picture_number =
>>> get_bits_long(gb, 32);
>> nit:
>>
>> picnum =
>> s->current_picture->avframe.display_picture_number = get_bits_long(gb,
>> 32);
>
> aargh, I dislike this one. Is it mandatory? it looks horrible to me
I do agree.
picnum = s->current_picture->avframe.display_picture_number =
get_bits_long(gb, 32);
lu
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
