On 08/06/17 12:35, Michael Niedermayer wrote:
> On Thu, Jun 08, 2017 at 11:02:30AM +0100, Mark Thompson wrote:
>> On 08/06/17 01:29, Jun Zhao wrote:
>>> V3: Clean the code logic base on Michael's review.
>>> V2: Add Add set_ue_golomb_long() to support 32bits UE golomb and update the
>>> unit test.
>>>
>>> From 4de3e0c30af7bc1901562000eda018a6d6849292 Mon Sep 17 00:00:00 2001
>>> From: Jun Zhao <jun.z...@intel.com>
>>> Date: Fri, 2 Jun 2017 15:05:49 +0800
>>> Subject: [PATCH V3] lavc/golomb: Fix UE golomb overwrite issue.
>>>
>>> put_bits just support write up to 31 bits, when write 32 bit in
>>> put_bits, it's will overwrite the bit buffer, because the default
>>> assert level is 0, the av_assert2(n <= 31 && value < (1U << n))
>>> in put_bits can not be trigger runtime. Add set_ue_golomb_long()
>>> to support 32bits UE golomb.
>>>
>>> Signed-off-by: Jun Zhao <jun.z...@intel.com>
> [...]
>>> +
>>> + if (i < 256)
>>> + put_bits(pb, ff_ue_golomb_len[i], i + 1);
>>> + else {
>>> + int e = av_log2(i + 1);
>>> + put_bits64(pb, 2 * e + 1, i + 1);
>>> + }
>>> +}
>>
>> Please don't add a new function, just extend the existing one.
>> set_ue_golomb() is not used in any place where minor ricing is of any value,
>> and having another function will only be confusing over which to use (or,
>> more likely, people will always use the long version in order to avoid
>> thinking about it just like they do with get_ue_golomb() - cf. "grep
>> get_ue_golomb libavcodec/hevc*").
>
> No question its easier to do something random than to think about
> the range allowed by the specification.
> But when writing data, its neccessary to comply to the specification,
> otherwise one creates invalid streams. So theres no way around thinking
> about the range even with just one function.
I don't understand what you're trying to say here. I'm suggesting that it
would be better to extend the range of values that set_ue_golomb() supports, so
that it can correctly write values up to 2^32-2. Since this is the largest
value H.264 and H.265 ever require, any use with those standards is known to be
in-range and no further thought is needed. It is possible that some other
codec might want to use larger Exp-Golomb codes - if that happens then indeed
more thought will be required when implementing that codec, but it shouldn't
affect anyone else.
- Mark
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
