On 13/01/17 11:31, Anton Khirnov wrote:
> Quoting Mark Thompson (2017-01-10 00:51:10)
>> From: "Ronald S. Bultje" <rsbul...@gmail.com>
>>
>> From ffmpeg commit 2e6636aa87303d37b112e79f093ca39500f92364.
>> ---
>> Exactly as in the other tine (except licence header). It might be nicer to
>> call it vp9_superframe_merge, but maintaining identical behaviour probably
>> trumps that?
>>
>>
>> libavcodec/Makefile | 1 +
>> libavcodec/bitstream_filters.c | 1 +
>> libavcodec/vp9_superframe_bsf.c | 205
>> ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 207 insertions(+)
>> create mode 100644 libavcodec/vp9_superframe_bsf.c
>>
>
> Some docs would be really nice. And a changelog entry.
I was trying to merge this exactly as the original patch to preserve correct
attribution (it does work as-is, even if I don't much like the code).
Some changes below, nontrivial ones should be in a second patch.
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index b3cee1d0c..7be9c2880 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -769,6 +769,7 @@ OBJS-$(CONFIG_NOISE_BSF) += noise_bsf.o
>> OBJS-$(CONFIG_NULL_BSF) += null_bsf.o
>> OBJS-$(CONFIG_REMOVE_EXTRADATA_BSF) += remove_extradata_bsf.o
>> OBJS-$(CONFIG_TEXT2MOVSUB_BSF) += movsub_bsf.o
>> +OBJS-$(CONFIG_VP9_SUPERFRAME_BSF) += vp9_superframe_bsf.o
>> OBJS-$(CONFIG_VP9_SUPERFRAME_SPLIT_BSF) += vp9_superframe_split_bsf.o
>>
>> # thread libraries
>> diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c
>> index 1cea6d77a..d46fdad81 100644
>> --- a/libavcodec/bitstream_filters.c
>> +++ b/libavcodec/bitstream_filters.c
>> @@ -38,6 +38,7 @@ extern const AVBitStreamFilter ff_null_bsf;
>> extern const AVBitStreamFilter ff_text2movsub_bsf;
>> extern const AVBitStreamFilter ff_noise_bsf;
>> extern const AVBitStreamFilter ff_remove_extradata_bsf;
>> +extern const AVBitStreamFilter ff_vp9_superframe_bsf;
>> extern const AVBitStreamFilter ff_vp9_superframe_split_bsf;
>>
>> #include "libavcodec/bsf_list.c"
>> diff --git a/libavcodec/vp9_superframe_bsf.c
>> b/libavcodec/vp9_superframe_bsf.c
>> new file mode 100644
>> index 000000000..27445176b
>> --- /dev/null
>> +++ b/libavcodec/vp9_superframe_bsf.c
>> @@ -0,0 +1,205 @@
>> +/*
>> + * Vp9 invisible (alt-ref) frame to superframe merge bitstream filter
>> + * Copyright (c) 2016 Ronald S. Bultje <rsbul...@gmail.com>
>> + *
>> + * This file is part of Libav.
>> + *
>> + * Libav is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * Libav is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with Libav; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
>> USA
>> + */
>> +
>> +#include "libavutil/avassert.h"
>> +#include "avcodec.h"
>> +#include "bsf.h"
>> +#include "get_bits.h"
>> +
>> +#define MAX_CACHE 8
>> +typedef struct VP9BSFContext {
>> + int n_cache;
>> + struct CachedBuf {
>> + uint8_t *data;
>> + int size;
>> + } cache[MAX_CACHE];
>> +} VP9BSFContext;
>> +
>> +static void stats(const struct CachedBuf *in, int n_in,
>> + unsigned *_max, unsigned *_sum)
>> +{
>> + int n;
>> + unsigned max = 0, sum = 0;
>> +
>> + for (n = 0; n < n_in; n++) {
>> + unsigned sz = in[n].size;
>> +
>> + if (sz > max)
>> + max = sz;
>> + sum += sz;
>> + }
>> +
>> + *_max = max;
>> + *_sum = sum;
>> +}
>> +
>> +static int merge_superframe(const struct CachedBuf *in, int n_in, AVPacket
>> *out)
>> +{
>> + unsigned max, sum, mag, marker, n, sz;
>> + uint8_t *ptr;
>> + int res;
>> +
>> + stats(in, n_in, &max, &sum);
>> + mag = av_log2(max) >> 3;
>> + marker = 0xC0 + (mag << 3) + (n_in - 1);
>> + sz = sum + 2 + (mag + 1) * n_in;
>> + res = av_new_packet(out, sz);
>> + if (res < 0)
>> + return res;
>> + ptr = out->data;
>> + for (n = 0; n < n_in; n++) {
>> + memcpy(ptr, in[n].data, in[n].size);
>> + ptr += in[n].size;
>> + }
>> +
>> +#define wloop(mag, wr) \
>> + for (n = 0; n < n_in; n++) { \
>> + wr; \
>> + ptr += mag + 1; \
>> + }
>> +
>> + // write superframe with marker 110[mag:2][nframes:3]
>> + *ptr++ = marker;
>> + switch (mag) {
>> + case 0:
>> + wloop(mag, *ptr = in[n].size);
>> + break;
>> + case 1:
>> + wloop(mag, AV_WL16(ptr, in[n].size));
>> + break;
>> + case 2:
>> + wloop(mag, AV_WL24(ptr, in[n].size));
>> + break;
>> + case 3:
>> + wloop(mag, AV_WL32(ptr, in[n].size));
>> + break;
>> + }
>> + *ptr++ = marker;
>> + av_assert0(ptr == &out->data[out->size]);
>> +
>> + return 0;
>> +}
>> +
>> +static int vp9_superframe_filter(AVBSFContext *ctx, AVPacket *out)
>> +{
>> + GetBitContext gb;
>
> Should switch to the new bitstream reader I suppose.
Sure, in a following patch.
>> + VP9BSFContext *s = ctx->priv_data;
>> + AVPacket *in;
>> + int res, invisible, profile, marker, uses_superframe_syntax = 0, n;
>> +
>> + res = ff_bsf_get_packet(ctx, &in);
>> + if (res < 0)
>> + return res;
>> +
>> + marker = in->data[in->size - 1];
>> + if ((marker & 0xe0) == 0xc0) {
>> + int nbytes = 1 + ((marker >> 3) & 0x3);
>> + int n_frames = 1 + (marker & 0x7), idx_sz = 2 + n_frames * nbytes;
>> +
>> + uses_superframe_syntax = in->size >= idx_sz && in->data[in->size -
>> idx_sz] == marker;
>> + }
>> +
>> + if ((res = init_get_bits8(&gb, in->data, in->size)) < 0)
>> + goto done;
>> +
>> + get_bits(&gb, 2); // frame marker
>> + profile = get_bits1(&gb);
>> + profile |= get_bits1(&gb) << 1;
>> + if (profile == 3) profile += get_bits1(&gb);
>> +
>> + if (get_bits1(&gb)) {
>> + invisible = 0;
>> + } else {
>> + get_bits1(&gb); // keyframe
>> + invisible = !get_bits1(&gb);
>> + }
>> +
>> + if (uses_superframe_syntax && s->n_cache > 0) {
>> + av_log(ctx, AV_LOG_ERROR,
>> + "Mixing of superframe syntax and naked VP9 frames not
>> supported");
>> + res = AVERROR_INVALIDDATA;
>
> nit: ENOSYS would be more appropriate, AFAIU the bitstream is not
> invalid in this case
I agree, changed.
>> + goto done;
>> + } else if ((!invisible || uses_superframe_syntax) && !s->n_cache) {
>> + // passthrough
>> + av_packet_move_ref(out, in);
>> + goto done;
>> + } else if (s->n_cache + 1 >= MAX_CACHE) {
>> + av_log(ctx, AV_LOG_ERROR,
>> + "Too many invisible frames");
>> + res = AVERROR_INVALIDDATA;
>> + goto done;
>> + }
>> +
>> + s->cache[s->n_cache].size = in->size;
>
> A bit premature to do this I think, on an error below we leave a stale
> size in there, which is not good.
Yeah. Moved after each copy.
>> + if (invisible && !uses_superframe_syntax) {
>
> The second condition is superfluous, no? If I'm reading correctly, it can
> never be true here.
True, removed.
>> + s->cache[s->n_cache].data = av_malloc(in->size);
>> + if (!s->cache[s->n_cache].data) {
>> + res = AVERROR(ENOMEM);
>> + goto done;
>> + }
>> + memcpy(s->cache[s->n_cache++].data, in->data, in->size);
>
> Feels a bit wasteful to do all those extra mallocs and copies, when you
> can just store the input packet at no cost. It's probably even less
> code.
Maybe later?
Thanks,
- Mark
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
