Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2604323002:
Fixing peerconnection reddish video issue (Closed)
DescriptionFixing peerconnection reddish video issue
BUG=webrtc:6857
NOTRY=True
Review-Url: https://codereview.webrtc.org/2604323002
Cr-Commit-Position: refs/heads/master@{#16143}
Committed: https://chromium.googlesource.com/external/webrtc/+/2f67b82e208552ce6aa5bd7d51cc67898a3bca9b
Patch Set 1 #
Total comments: 3
Patch Set 2 : Adding a comment to the calling site of I420ToABGR #Patch Set 3 : Rebasing #Messages
Total messages: 31 (12 generated)
mbonadei@webrtc.org changed reviewers: + kjellander@webrtc.org
Ok, this fixes the reddish issue (I did a manual test).
Thanks mirko, the failing ios32_sim_dbg and ios64_sim_dbg trybots are due to me renaming the bots; you'll have to update your alias. lgtm
The CQ bit was checked by mbonadei@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11823)
mbonadei@webrtc.org changed reviewers: + tommi@webrtc.org
Adding tommi@ for the lgtm from an owner of the code.
mbonadei@webrtc.org changed reviewers: + magjed@webrtc.org
Adding also magjed@ for the lgtm from an owner.
https://codereview.webrtc.org/2604323002/diff/1/webrtc/examples/peerconnectio... File webrtc/examples/peerconnection/client/linux/main_wnd.cc (right): https://codereview.webrtc.org/2604323002/diff/1/webrtc/examples/peerconnectio... webrtc/examples/peerconnection/client/linux/main_wnd.cc:531: libyuv::I420ToABGR(buffer->DataY(), buffer->StrideY(), hmm... that's strange. I don't think this has changed in a long while and the regression sounds like something new. Doesn't the image buffer end up being used by gdk_draw_rgb_32_image? Maybe there's something wrong with the scaling code?
+nisse regarding https://chromium.googlesource.com/external/webrtc/+/9f8e37b6f833cb6c821c6f401... https://codereview.webrtc.org/2604323002/diff/1/webrtc/examples/peerconnectio... File webrtc/examples/peerconnection/client/linux/main_wnd.cc (right): https://codereview.webrtc.org/2604323002/diff/1/webrtc/examples/peerconnectio... webrtc/examples/peerconnection/client/linux/main_wnd.cc:531: libyuv::I420ToABGR(buffer->DataY(), buffer->StrideY(), On 2017/01/02 15:37:49, tommi (webrtc) wrote: > hmm... that's strange. I don't think this has changed in a long while and the > regression sounds like something new. Doesn't the image buffer end up being > used by gdk_draw_rgb_32_image? Maybe there's something wrong with the scaling > code? You're right we should probably look into what caused this. According to https://chromium.googlesource.com/external/webrtc/+blame/master/webrtc/exampl... it was changed as recently as Sep 1 by nisse in https://chromium.googlesource.com/external/webrtc/+/9f8e37b6f833cb6c821c6f401... Mirko: can you revert that CL locally and see if it solves the problem?
https://codereview.webrtc.org/2604323002/diff/1/webrtc/examples/peerconnectio... File webrtc/examples/peerconnection/client/linux/main_wnd.cc (right): https://codereview.webrtc.org/2604323002/diff/1/webrtc/examples/peerconnectio... webrtc/examples/peerconnection/client/linux/main_wnd.cc:531: libyuv::I420ToABGR(buffer->DataY(), buffer->StrideY(), On 2017/01/03 09:24:26, kjellander_webrtc wrote: > On 2017/01/02 15:37:49, tommi (webrtc) wrote: > > hmm... that's strange. I don't think this has changed in a long while and the > > regression sounds like something new. Doesn't the image buffer end up being > > used by gdk_draw_rgb_32_image? Maybe there's something wrong with the scaling > > code? > > You're right we should probably look into what caused this. According to > https://chromium.googlesource.com/external/webrtc/+blame/master/webrtc/exampl... > it was changed as recently as Sep 1 by nisse in > https://chromium.googlesource.com/external/webrtc/+/9f8e37b6f833cb6c821c6f401... > Mirko: can you revert that CL locally and see if it solves the problem? Yes, I checked out the commit before and the video was not reddish. The first commit which exposes this behaviour is 9f8e37b6f833cb6c821c6f401421b7c33cbe4b27.
tommi@webrtc.org changed reviewers: + nisse@webrtc.org
Nisse - can you take a look at your original CL and see what the issue is?
On 2017/01/08 18:33:34, tommi (webrtc) wrote: > Nisse - can you take a look at your original CL and see what the issue is? My change is here: https://codereview.webrtc.org/2287233002/diff/120001/webrtc/examples/peerconn... I'm confused by this byte-order juggling. Comments on the old code weren't quite consistent, and docs https://developer.gnome.org/gdk2/stable/gdk2-GdkRGB.html#gdk-draw-rgb-32-image say that RGBA is right (with the A byte ignored). Looking at the libyuv code, it looks like I420ToRGBA produces ABGR, not RGBA, but I'm not really sure. Confusing, indeed. Anyway, if this cl fixes the problem, that's good. LGTM.
On 2017/01/09 09:40:19, nisse-webrtc wrote: > On 2017/01/08 18:33:34, tommi (webrtc) wrote: > > Nisse - can you take a look at your original CL and see what the issue is? > > My change is here: > https://codereview.webrtc.org/2287233002/diff/120001/webrtc/examples/peerconn... > > I'm confused by this byte-order juggling. Comments on the old code weren't quite > consistent, and docs > https://developer.gnome.org/gdk2/stable/gdk2-GdkRGB.html#gdk-draw-rgb-32-image > say that RGBA is right (with the A byte ignored). > > Looking at the libyuv code, it looks like I420ToRGBA produces ABGR, not RGBA, > but I'm not really sure. Confusing, indeed. > > Anyway, if this cl fixes the problem, that's good. LGTM. Magnus: do you know if I understood the libyuv code correctly, and that the libyuv I420ToRGBA function seems to produce a different byte order than expected (i.e., ABGR, byte-swapped)?
On 2017/01/09 15:14:55, nisse-webrtc wrote: > On 2017/01/09 09:40:19, nisse-webrtc wrote: > > On 2017/01/08 18:33:34, tommi (webrtc) wrote: > > > Nisse - can you take a look at your original CL and see what the issue is? > > > > My change is here: > > > https://codereview.webrtc.org/2287233002/diff/120001/webrtc/examples/peerconn... > > > > I'm confused by this byte-order juggling. Comments on the old code weren't > quite > > consistent, and docs > > https://developer.gnome.org/gdk2/stable/gdk2-GdkRGB.html#gdk-draw-rgb-32-image > > say that RGBA is right (with the A byte ignored). > > > > Looking at the libyuv code, it looks like I420ToRGBA produces ABGR, not RGBA, > > but I'm not really sure. Confusing, indeed. > > > > Anyway, if this cl fixes the problem, that's good. LGTM. > > Magnus: do you know if I understood the libyuv code correctly, and that the > libyuv I420ToRGBA function seems to produce a different byte order than expected > (i.e., ABGR, byte-swapped)? The order in the naming is ambiguous because you never know if it's referring to how it's laid out in memory as bytes, or if endianness is taken into account. All I can say is that libyuv is using an internally consistent naming, and that it's best to just test your changes and see if the colors show up correctly, so please look over your original CL and see if you have missed other places.
On 2017/01/10 10:40:43, magjed_webrtc wrote: > On 2017/01/09 15:14:55, nisse-webrtc wrote: > > On 2017/01/09 09:40:19, nisse-webrtc wrote: > > > On 2017/01/08 18:33:34, tommi (webrtc) wrote: > > > > Nisse - can you take a look at your original CL and see what the issue is? > > > > > > My change is here: > > > > > > https://codereview.webrtc.org/2287233002/diff/120001/webrtc/examples/peerconn... > > > > > > I'm confused by this byte-order juggling. Comments on the old code weren't > > quite > > > consistent, and docs > > > > https://developer.gnome.org/gdk2/stable/gdk2-GdkRGB.html#gdk-draw-rgb-32-image > > > say that RGBA is right (with the A byte ignored). > > > > > > Looking at the libyuv code, it looks like I420ToRGBA produces ABGR, not > RGBA, > > > but I'm not really sure. Confusing, indeed. > > > > > > Anyway, if this cl fixes the problem, that's good. LGTM. > > > > Magnus: do you know if I understood the libyuv code correctly, and that the > > libyuv I420ToRGBA function seems to produce a different byte order than > expected > > (i.e., ABGR, byte-swapped)? > > The order in the naming is ambiguous because you never know if it's referring to > how it's laid out in memory as bytes, or if endianness is taken into account. > All I can say is that libyuv is using an internally consistent naming, and that > it's best to just test your changes and see if the colors show up correctly, so > please look over your original CL and see if you have missed other places. In my opinion, since I420ToRGBA uses a byte pointer (not an uint32_t*) for the destination argument, it makes no sense that it refers to anything but layout of bytes in memory. But we can't do much about that, at least not in this cl. Do you know if I420ToRGBA at least produces the same order of bytes in memory regardless of endianness? For this cl, I guess all we need is a clear comment at the call site, to explain what's going on. In my original cl, I think this is the only place, and that change was done to eliminate an additional pass over the data to swap bytes around.
The CQ bit was checked by mbonadei@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org, nisse@webrtc.org Link to the patchset: https://codereview.webrtc.org/2604323002/#ps40001 (title: "Rebasing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12387)
lgtm
The CQ bit was checked by mbonadei@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484742063827460,
"parent_rev": "5850a9484d21926f8c71c7ff899f3548d55ff794", "commit_rev":
"2f67b82e208552ce6aa5bd7d51cc67898a3bca9b"}
Message was sent while issue was closed.
Description was changed from ========== Fixing peerconnection reddish video issue BUG=webrtc:6857 NOTRY=True ========== to ========== Fixing peerconnection reddish video issue BUG=webrtc:6857 NOTRY=True Review-Url: https://codereview.webrtc.org/2604323002 Cr-Commit-Position: refs/heads/master@{#16143} Committed: https://chromium.googlesource.com/external/webrtc/+/2f67b82e208552ce6aa5bd7d5... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/2f67b82e208552ce6aa5bd7d5... |
