On 10/10/2013 03:19 PM, Vittorio Giovara wrote: > On Thu, Oct 10, 2013 at 11:37 PM, John Stebbins <[email protected]> > wrote: >> On 10/10/2013 01:48 PM, Vittorio Giovara wrote: >>> On Thursday, October 10, 2013, John Stebbins wrote: >>> >>>> This can be optionally disabled whith the "showall" flags2 option. >>>> When in "showall" mode, incomplete frames are signalled through >>>> AVFrame.flags FRAME_FLAG_INCOMPLETE_FRAME. >>>> diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h >>>> index 81e3d2b..48b3cae 100644 >>>> --- a/libavcodec/mpegvideo.h >>>> +++ b/libavcodec/mpegvideo.h >>>> @@ -172,6 +172,7 @@ typedef struct Picture{ >>>> >>>> int reference; >>>> int shared; >>>> + int recovered; ///< Picture at IDR or recovery point + >>>> recovery count >>>> } Picture; >>> I'm not sure I understand why this is needed here, AVFrame seems enough to >>> store recovery information no? >> AVFrame is a public struct. Applications don't need to see this internal >> state tracking data. > Yes, umh so if this info is already in avframe why does it need to be > in mpegvideo?
Huh? Where is it in AVFrame? I didn't add it there. I don't see why anyone
else would have added it there.
>>>
>>>> /**
>>>> diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
>>>> index 5e9d484..9113272 100644
>>>> --- a/libavcodec/options_table.h
>>>> +++ b/libavcodec/options_table.h
>>>> @@ -72,6 +72,7 @@ static const AVOption avcodec_options[] = {
>>>> {"noout", "skip bitstream encoding", 0, AV_OPT_TYPE_CONST, {.i64 =
>>>> CODEC_FLAG2_NO_OUTPUT }, INT_MIN, INT_MAX, V|E, "flags2"},
>>>> {"ignorecrop", "ignore cropping information from sps", 1,
>>>> AV_OPT_TYPE_CONST, {.i64 = CODEC_FLAG2_IGNORE_CROP }, INT_MIN, INT_MAX,
>>>> V|D, "flags2"},
>>>> {"local_header", "place global headers at every keyframe instead of in
>>>> extradata", 0, AV_OPT_TYPE_CONST, {.i64 = CODEC_FLAG2_LOCAL_HEADER },
>>>> INT_MIN, INT_MAX, V|E, "flags2"},
>>>> +{"showall", "Show all frames before the first keyframe", 0,
>>>> AV_OPT_TYPE_CONST, {.i64 = CODEC_FLAG2_SHOW_ALL }, INT_MIN, INT_MAX, V|D,
>>>> "flags2"},
>>>> {"me_method", "set motion estimation method", OFFSET(me_method),
>>>> AV_OPT_TYPE_INT, {.i64 = ME_EPZS }, INT_MIN, INT_MAX, V|E, "me_method"},
>>>> {"zero", "zero motion estimation (fastest)", 0, AV_OPT_TYPE_CONST, {.i64
>>>> = ME_ZERO }, INT_MIN, INT_MAX, V|E, "me_method" },
>>>> {"full", "full motion estimation (slowest)", 0, AV_OPT_TYPE_CONST, {.i64
>>>> = ME_FULL }, INT_MIN, INT_MAX, V|E, "me_method" },
>>>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>>>> index 098bbed..78bad66 100644
>>>> --- a/libavutil/frame.c
>>>> +++ b/libavutil/frame.c
>>>> @@ -379,6 +379,7 @@ int av_frame_copy_props(AVFrame *dst, const AVFrame
>>>> *src)
>>>> dst->quality = src->quality;
>>>> dst->coded_picture_number = src->coded_picture_number;
>>>> dst->display_picture_number = src->display_picture_number;
>>>> + dst->flags = src->flags;
>>>>
>>>> memcpy(dst->error, src->error, sizeof(dst->error));
>>>>
>>>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>>>> index b0676e7..19fdc1b 100644
>>>> --- a/libavutil/frame.h
>>>> +++ b/libavutil/frame.h
>>>> @@ -350,6 +350,9 @@ typedef struct AVFrame {
>>>>
>>>> AVFrameSideData **side_data;
>>>> int nb_side_data;
>>>> +
>>>> + int flags;
>>>> +#define FRAME_FLAG_INCOMPLETE_FRAME 0x0001
>>>> } AVFrame;
>>> I adding the flag here breaks ABI; this kind on data should go in side_data
>>> in my opinion. Also you always set this flag but never read it, is this the
>>> intended behavior?
>> This is information that may be useful to an application. It is not needed
>> by libav. It can be eliminated if you all
>> feel strongly that you don't want it. But I think an application that
>> chooses to enable the "showall" feature may also
>> be interested in which frames are actually complete and which are not.
> That's a nice purpose and should be available, as long as it doesn't
> break ABI though.
>
>> I haven't looked into the design of side_data,
>> so I don't know if it is appropriate to put this flag there. If someone who
>> understands the side_data design thinks
>> this is an architecturally sound thing to do, I would gladly move it there.
> It's quite easy, take a look at https://patches.libav.org/patch/42767/
> All you need is define a new element in AVFrameSideData
> (libavutil/frame.h) and then access it like in the patch above.
Thanks. I'll have a look and rework the patch to use side data.
--
John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
signature.asc
Description: OpenPGP digital signature
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
