flypig.co.uk

I'm moving on today from crashing video to discolouration of video. Yesterday I was, I think, able to fix the crash by ensuring only a single instance of GLLibraryEGL is created during execution of the browser. Today the problem is different: the question today is why is the browser confusing green, blue and red channels with luma, blue difference chroma and red difference chroma channels?

On Mastodon Vlad G (vlagged) came up with the nice suggestion of amending the environment variables used for configuring gecko-camera. Vlad has had previous experience with browser video playback issues from earlier versions of Sailfish OS and has also always been a big supporter of this gecko upgrade process, so I'm keen to give the suggestion a try. Here's Vlad's comment:
 
We can see the code that reads in these environment variables in the gecko-camera source on GitHub, although we have to dig a little deeper to determine what they actually do.

As Vlad points out, each of the two environment variables can take one of three different values: unset, 0 or 1, giving a total of nine potential variations. I tested all nine of them, but unfortunately all give the same results: the video discolouration remains.

Nevertheless, the suggestion led me to the gecko-camera code. There have also been some useful comments from Frajo (krnlyng) and Björn (Thaodan) on the related GitHub issue. These point out that the Y'CbCr buffer may not be being initialised correctly:
 

These comments relate to WebRTC which follows a different execution flow. Nevertheless they made me think about the way the buffer is being set up in the gecko code. The crucial work seems to happen inside the GeckoCameraVideoDecoder::onDecodedYCbCrFrame() method in the GeckoCameraVideoDecoder.cpp source file. There we see the following code:

  VideoData::YCbCrBuffer buffer;
  // Y plane.
  buffer.mPlanes[0].mData = const_cast<uint8_t*>(frame->y);
  buffer.mPlanes[0].mStride = frame->yStride;
  buffer.mPlanes[0].mWidth = frame->width;
  buffer.mPlanes[0].mHeight = frame->height;
  buffer.mPlanes[0].mSkip = 0;
  // Cb plane.
  buffer.mPlanes[1].mData = const_cast<uint8_t*>(frame->cb);
  buffer.mPlanes[1].mStride = frame->cStride;
  buffer.mPlanes[1].mWidth = (frame->width + 1) / 2;
  buffer.mPlanes[1].mHeight = (frame->height + 1) / 2;
  buffer.mPlanes[1].mSkip = frame->chromaStep - 1;
  // Cr plane.
  buffer.mPlanes[2].mData = const_cast<uint8_t*>(frame->cr);
  buffer.mPlanes[2].mStride = frame->cStride;
  buffer.mPlanes[2].mWidth = (frame->width + 1) / 2;
  buffer.mPlanes[2].mHeight = (frame->height + 1) / 2;
  buffer.mPlanes[2].mSkip = frame->chromaStep - 1;

This takes all of the input channels, structures them for the underlying video renderer and passes them on to be rendered. If we look at the definition of the VideoData::YCbCrBuffer structure that's being filled out, we find there are a few fields that are being left unset. Here's the structure as it's defined in MediaData.h:

  // YCbCr data obtained from decoding the video. The index's are:
  //   0 = Y
  //   1 = Cb
  //   2 = Cr
  struct YCbCrBuffer {
    struct Plane {
      uint8_t* mData;
      uint32_t mWidth;
      uint32_t mHeight;
      uint32_t mStride;
      uint32_t mSkip;
    };

    Plane mPlanes[3];
    YUVColorSpace mYUVColorSpace = YUVColorSpace::Identity;
    ColorDepth mColorDepth = ColorDepth::COLOR_8;
    ColorRange mColorRange = ColorRange::LIMITED;
  };

In the decoder code we can see the mPlanes substructure being filled out nicely, but the other three fields, mYUVColorSpace, mColorDepth and mColorRange are simply being left as their default values.

Perhaps we'd have better results if these were set to something. Looking at other decoders for other platforms and services, it seems that at least the mYUVColorSpace and mColorRange are routinely being filled out by the decoder.

But what values to give them? To try to understand better I thought it could be helpful to look at the description of the commits that added them. It turns out each of the three remaining variables were each added at different times:

$ git blame dom/media/MediaData.h -L 431,448
Blaming lines:   2% (18/696), done.
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 431)
   // YCbCr data obtained from decoding the video. The index's are:
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 432)
   //   0 = Y
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 433)
   //   1 = Cb
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 434)
   //   2 = Cr
804b8b8883ba2 dom/media/MediaData.h     (Sylvestre Ledru   2018-11-19 435)
   struct YCbCrBuffer {
804b8b8883ba2 dom/media/MediaData.h     (Sylvestre Ledru   2018-11-19 436)
     struct Plane {
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 437)
       uint8_t* mData;
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 438)
       uint32_t mWidth;
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 439)
       uint32_t mHeight;
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 440)
       uint32_t mStride;
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 441)
       uint32_t mSkip;
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 442)
     };
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 443)
 
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 444)
     Plane mPlanes[3];
d517313a4029f dom/media/MediaData.h     (Jeff Gilbert      2021-03-19 445)
     YUVColorSpace mYUVColorSpace = YUVColorSpace::Identity;
b10364a15f063 dom/media/MediaData.h     (Jean-Yves Avenard 2018-09-25 446)
     ColorDepth mColorDepth = ColorDepth::COLOR_8;
bb8acbc1f9c3e dom/media/MediaData.h     (Jean-Yves Avenard 2019-07-26 447)
     ColorRange mColorRange = ColorRange::LIMITED;
3e4230f663e4e content/media/MediaData.h (Ben Kelly         2014-02-03 448)
   };

Here are the three relevant commits. First the commit that added the mYUVColorSpace variable:

$ git log -1 d517313a4029f
commit d517313a4029f626220a7cdc2a6a80461ee37490
Author: Jeff Gilbert <jgilbert@mozilla.com>
Date:   Fri Mar 19 00:58:23 2021 +0000

    Bug 1697670 - Remove gfx::YUVColorSpace::UNKNOWN. r=mstange
    
    Replace with Maybe<YUVColorSpace> where still needed.
    
    Differential Revision: https://phabricator.services.mozilla.com/D107938

It looks like setting this value to the default might make a good choice. To do this I've followed the approach used in some other decoders which seem to use the width and height as parameters. Next we have the code change that added the mColorDepth variable. This seems to be more straightforward, and in our case I'm fairly certain we're using 8-bit colour.

$ git log -1 b10364a15f063
commit b10364a15f06371361a0e404c7f4a0d32f7c81d2
Author: Jean-Yves Avenard <jyavenard@mozilla.com>
Date:   Tue Sep 25 20:44:55 2018 +0000

    Bug 1493198 - P2. Use enum for describing color depth. r=mattwoodrow
    
    Depends on D6662
    
    Differential Revision: https://phabricator.services.mozilla.com/D6663
    
    --HG--
    extra : moz-landing-system : lando

Finally the change that introduced mColorRange:

$ git log -1 bb8acbc1f9c3e
commit bb8acbc1f9c3eee5a6a282e356ce863bc4ddae7d
Author: Jean-Yves Avenard <jyavenard@mozilla.com>
Date:   Fri Jul 26 08:45:37 2019 +0000

    Bug 1543359 - P1. Add mColorRange info to YCbCrBufferData. r=bryce
    
    Differential Revision: https://phabricator.services.mozilla.com/D27210
    
    --HG--
    extra : moz-landing-system : lando

Looking at this last commit doesn't really give me an answer, so again I'm going to try sticking to the default. The result of all this is that I've added the following three lines to the onDecodedYCbCrFrame() method to update the structure before it gets passed on to the renderer:

  buffer.mYUVColorSpace = DefaultColorSpace({frame->width, frame->height});
  buffer.mColorDepth = gfx::ColorDepth::COLOR_8;
  buffer.mColorRange = gfx::ColorRange::LIMITED;

On testing this out I find that... hooray!... the video now looks perfect. The colours are no longer mangled and the quality looks pretty good to me. Unfortunately the feed coming from the actual camera when using WebRTC isn't fixed, but video playback is now working nicely.

Although the camera feed discolouration persists, my aim here was to fix the audio and video playback. Both are now working correctly and I'm happy with the results. So I'm ticking these two items from the testing list.

That means the list is complete and I can now move on to other tasks. I have two significant remaining tasks. One is to get the page width interpreted correctly. The second is to get the browser working on Sailfish OS 4.6. I don't yet know where to start with either but I feel like I've already done enough for today, so I'll leave these as tasks for tomorrow.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.