[FFmpeg-devel] [PATCH]: libavcodec/webp

[Pascal Massimino]

Hi Reimar,

On Thu, Sep 18, 2014 at 12:28 PM, Reimar Döffinger <Reimar.Doeffinger at gmx.de
> wrote:

> On 18 September 2014 10:55:00 CEST, Pascal Massimino <
> pascal.massimino at gmail.com> wrote:
> >Hi Reimar,
> >
> >On Thu, Sep 18, 2014 at 9:33 AM, Reimar Döffinger
> ><Reimar.Doeffinger at gmx.de>
> >wrote:
> >
> >> On 18.09.2014, at 08:45, Pascal Massimino
> ><pascal.massimino at gmail.com>
> >> wrote:
> >> > Hi,
> >> >
> >> > the webp lossless doc was unclear regarding palette index falling
> >out of
> >> > range.
> >> > See issue #206 [1] for the bug report.
> >> > The solution retained was to treat out-of-range index as color
> >> 0x00000000,
> >> > so we could keep the speed-up in libwebp (we use fake extra entries
> >in
> >> the
> >> > cmap to handle out-of-bound values without the extra branch).
> >> >
> >> > The doc was clarified (along with few other loose ends) in patch
> >#71605
> >> [2]
> >> > Attached is the fix for ffmpeg's webp decoder to treat out-of-bound
> >index
> >> > as transparent-black instead of reporting an error.
> >>
> >> The spec now says "should be set". I don't fully understand why we
> >would
> >> ordinarily expect out-of-range value.
> >>
> >
> >corrupt files, for instance. Or just malicious ones.
>
> Why in all the world would you make handling of corrupted and malicious
> files part of the spec?
> That certainly should be left up to the decoder!
> A safety-oriented or conformance checking one would immediately abort, a
> speed oriented one would do whatever is fastest and a data recovery one
> would use advanced error resilience methods.
> Why would it be desirable for all of them to be forced to do the same
> thing?
>
> >I also don't understand why the issue talks about speed loss in the
> >> decoder, when it is the encoder that decides to use out-of-bounds
> >values.
> >>
> >
> >I considered both options: making the values forbidden, or defining a
> >behaviour for out-of-bound.
> >It turned out (see bug issue text) that introducing the bound-check in
> >an
> >already tight-loop was slowing down decoding up to 9%.
> >All things considered, pragmatism indicates that the latter option
> >should
> >be favored.
>
> Sorry that makes no sense.
> If an input is invalid you decoder can do whatever it wants.
> Declaring something as invalid in the spec purely logically can in no way
> cause a slowdown.

Now for libwebp you might have some issues if you insist that you want it
> to be both reference decoder and fast at the same time. But that's an issue
> with your choice of implementing things, not the spec.

With the explanation you gave, I would say both your change to the spec and
> the proposed FFmpeg change are just nonsense.
>
>
while i don't necessarily disagree with the above generally speaking [*],
let me repeat that
this is a pragmatic choice. I'm not going to give up on the speed-up i have
right now for
some hypothetical decoder later. This was facilitated by the fact that the
change was minor
and innocuous for ffmpeg.

/skal

(on a tangential side, this change also saves 2-6 bytes for files that
actually have 0x00000000
color in their palette, somewhat frequent case, because you then don't need
to transmit this color)

[*] just, it sounds like an MPEG-committee discussion!