kleopatra999 / webm

Automatically exported from code.google.com/p/webm
0 stars 0 forks source link

possible incorrect flag test for FORCE_KF #885

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Bruce reports this issue from the Chromium code analysis:
https://code.google.com/p/chromium/issues/detail?id=427616

In the following change, the | comparison in the if statement is suspect:

commit 48a762731695c14d3810f0396b5aa53ba0c725dc
Author: Deb Mukherjee <debargha@google.com>
Date:   Tue Nov 11 16:09:07 2014 -0800

    Vidyo: Turn off keyframes in higher spatial layers

    Change-Id: Icdd5e71cd6a2b59bc4b3b972af9e4d4a36821792

diff --git a/vp9/encoder/vp9_encoder.c b/vp9/encoder/vp9_encoder.c
index 72e11d6..fd7c974 100644
--- a/vp9/encoder/vp9_encoder.c
+++ b/vp9/encoder/vp9_encoder.c
@@ -3635,6 +3635,13 @@ int vp9_get_compressed_data(VP9_COMP *cpi, unsigned int 
*frame_flags,
     if (source != NULL) {
       cm->show_frame = 1;
       cm->intra_only = 0;
+      // if the flags indicate intra frame, but if the current picture is for
+      // non-zero spatial layer, it should not be an intra picture.
+      // TODO(Won Kap): this needs to change if per-layer intra frame is
+      // allowed.
+      if ((source->flags | VPX_EFLAG_FORCE_KF) && cpi->svc.spatial_layer_id) {
+        source->flags &= ~(unsigned int)(VPX_EFLAG_FORCE_KF);
+      }

       // Check to see if the frame should be encoded as an arf overlay.
       check_src_altref(cpi, source);

Bruce's notes:
I've been running VC++'s /analyze on Chrome and it pointed out a new bug in the 
vpx code base. The warning is:

d:\src\chromium\src\third_party\libvpx\source\libvpx\vp9\encoder\vp9_encoder.c(3
647) : warning C6316: Incorrect operator:  tested expression is constant and 
non-zero.  Use bitwise-and to determine whether bits are set.

It's not a particularly serious bug because all the code does if it thinks that 
the bit is set is clear it:

      if ((source->flags | VPX_EFLAG_FORCE_KF) && cpi->svc.spatial_layer_id) {
        source->flags &= ~(unsigned int)(VPX_EFLAG_FORCE_KF);
      }

The code seems a bit odd overall -- there's not really any value in testing 
whether a bit is set before zeroing it -- you might as well just zero it. 
Possible fixes would include:

      if ((source->flags & VPX_EFLAG_FORCE_KF) && cpi->svc.spatial_layer_id) {
        source->flags &= ~(unsigned int)(VPX_EFLAG_FORCE_KF);
      }

or:

      if (cpi->svc.spatial_layer_id) {
        source->flags &= ~(unsigned int)(VPX_EFLAG_FORCE_KF);
      }

Original issue reported on code.google.com by johannko...@google.com on 2 Dec 2014 at 6:27

GoogleCodeExporter commented 9 years ago
https://gerrit.chromium.org/gerrit/73139

Original comment by johannko...@google.com on 2 Dec 2014 at 11:53