Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
663183002:
ppapi: Change endianness in libyuv calls, from BGRA to ARGB (Closed)
|
Created:
6 years, 2 months ago by magjed_chromium Modified:
6 years, 2 months ago Reviewers:
fbarchard, tommi (sloooow) - chröme, brucedawson, tpsiaki, thorcarpenter, perkj_chrome, fbarchard1 CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDetect endianness automatically from the data received from the plugin.
BUG=426020
Committed: https://crrev.com/8cd330dced386cf0f3eb6888af8b9f46b90d8d1c
Cr-Commit-Position: refs/heads/master@{#301079}
Patch Set 1 #
Total comments: 2
Patch Set 2 : automatically detect endianness #
Total comments: 2
Patch Set 3 : Add todo and remove PepperVideoSourceHost from this CL #
Total comments: 2
Patch Set 4 : tpsiaki@s and thorcarpenter@s comments #
Total comments: 2
Patch Set 5 : clarifying comments #
Total comments: 8
Patch Set 6 : fbarchard@s comments #Messages
Total messages: 40 (8 generated)
magjed@chromium.org changed reviewers: + thorcarpenter@chromium.org, tommi@chromium.org, tpsiaki@google.com
Please take a look.
lgtm fwiw - this really needs to be vetted by someone familiar with libyuv and the fx plugin. Can you add a bug reference as well for tracking?
thorcarpenter@chromium.org changed reviewers: + fbarchard@google.com
lgtm Thanks for making this change. Before submitting though, please let us get a new NaCl plugin deployed so we don't break canary users.
fbarchard@chromium.org changed reviewers: + fbarchard@chromium.org
lgtm https://codereview.chromium.org/663183002/diff/1/content/renderer/media/webrt... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/663183002/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/video_destination_handler.cc:105: DCHECK(image_data->format() == PP_IMAGEDATAFORMAT_BGRA_PREMUL); FYI our effects code is normally unattenuated. https://codereview.chromium.org/663183002/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/video_destination_handler.cc:158: libyuv::ARGBToI420(data, note that this has immediate performance benefit - windows version will use AVX2, which is about 40% faster on haswell.
lgtm https://codereview.chromium.org/663183002/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/663183002/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:158: // Due to confusion about endianness, we try to determine it from the data. I like this, but consider adding TODO and referencing bug for when we can remove this and always do ARGBToI420. Probably m41 time frame.
Patchset #3 (id:40001) has been deleted
Can you please take another look? I have removed the change in PepperVideoSourceHost from this CL and put it in another CL: https://codereview.chromium.org/675513002/ This CL should now be compatible with any version, so we can commit it.
It shouldn't be necessary to do this every frame. Can we just do it once per stream?
(the endian check that is) On Wed, Oct 22, 2014 at 9:45 AM, <tpsiaki@google.com> wrote: > It shouldn't be necessary to do this every frame. Can we just do it once > per > stream? > > https://codereview.chromium.org/663183002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/663183002/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/663183002/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:188: (endian == XXXA) ? libyuv::ARGBToI420 : libyuv::BGRAToI420; can we do xxxToI420 = (endian == XXXA || endian == UNKNOWN) ? libyuv::ARGBToI420 : libyuv::BGRAToI420; Since ARGBToI420 is the future stable version of this code, and it's possible for this code to make a mistake in the case of a pure blue frame, let's error on the side of long term caution.
PTAL. I changed so that the endian is stored as a member variable. If we can figure it out once, we won't do any more checks. https://codereview.chromium.org/663183002/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/663183002/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:158: // Due to confusion about endianness, we try to determine it from the data. On 2014/10/21 21:14:48, thorcarpenter wrote: > I like this, but consider adding TODO and referencing bug for when we can remove > this and always do ARGBToI420. Probably m41 time frame. Done. https://codereview.chromium.org/663183002/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/663183002/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:188: (endian == XXXA) ? libyuv::ARGBToI420 : libyuv::BGRAToI420; On 2014/10/22 16:56:39, thorcarpenter wrote: > can we do > > xxxToI420 = (endian == XXXA || endian == UNKNOWN) ? libyuv::ARGBToI420 : > libyuv::BGRAToI420; > > Since ARGBToI420 is the future stable version of this code, and it's possible > for this code to make a mistake in the case of a pure blue frame, let's error on > the side of long term caution. Done.
perhaps gcl upload? I'm still seeing (endian_ == AXXX) On Wed, Oct 22, 2014 at 10:27 AM, <magjed@chromium.org> wrote: > PTAL. I changed so that the endian is stored as a member variable. If we > can > figure it out once, we won't do any more checks. > > > https://codereview.chromium.org/663183002/diff/20001/ > content/renderer/media/webrtc/video_destination_handler.cc > File content/renderer/media/webrtc/video_destination_handler.cc (right): > > https://codereview.chromium.org/663183002/diff/20001/ > content/renderer/media/webrtc/video_destination_handler.cc#newcode158 > content/renderer/media/webrtc/video_destination_handler.cc:158: // Due > to confusion about endianness, we try to determine it from the data. > On 2014/10/21 21:14:48, thorcarpenter wrote: > >> I like this, but consider adding TODO and referencing bug for when we >> > can remove > >> this and always do ARGBToI420. Probably m41 time frame. >> > > Done. > > https://codereview.chromium.org/663183002/diff/60001/ > content/renderer/media/webrtc/video_destination_handler.cc > File content/renderer/media/webrtc/video_destination_handler.cc (right): > > https://codereview.chromium.org/663183002/diff/60001/ > content/renderer/media/webrtc/video_destination_handler.cc#newcode188 > content/renderer/media/webrtc/video_destination_handler.cc:188: (endian > == XXXA) ? libyuv::ARGBToI420 : libyuv::BGRAToI420; > On 2014/10/22 16:56:39, thorcarpenter wrote: > >> can we do >> > > xxxToI420 = (endian == XXXA || endian == UNKNOWN) ? libyuv::ARGBToI420 >> > : > >> libyuv::BGRAToI420; >> > > Since ARGBToI420 is the future stable version of this code, and it's >> > possible > >> for this code to make a mistake in the case of a pure blue frame, >> > let's error on > >> the side of long term caution. >> > > Done. > > https://codereview.chromium.org/663183002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
oh, after rereading, it looks right. staring at too many endians is confusing :-) On Wed, Oct 22, 2014 at 10:47 AM, Thor Carpenter <thorcarpenter@google.com> wrote: > perhaps gcl upload? > > I'm still seeing (endian_ == AXXX) > > On Wed, Oct 22, 2014 at 10:27 AM, <magjed@chromium.org> wrote: > >> PTAL. I changed so that the endian is stored as a member variable. If we >> can >> figure it out once, we won't do any more checks. >> >> >> https://codereview.chromium.org/663183002/diff/20001/ >> content/renderer/media/webrtc/video_destination_handler.cc >> File content/renderer/media/webrtc/video_destination_handler.cc (right): >> >> https://codereview.chromium.org/663183002/diff/20001/ >> content/renderer/media/webrtc/video_destination_handler.cc#newcode158 >> content/renderer/media/webrtc/video_destination_handler.cc:158: // Due >> to confusion about endianness, we try to determine it from the data. >> On 2014/10/21 21:14:48, thorcarpenter wrote: >> >>> I like this, but consider adding TODO and referencing bug for when we >>> >> can remove >> >>> this and always do ARGBToI420. Probably m41 time frame. >>> >> >> Done. >> >> https://codereview.chromium.org/663183002/diff/60001/ >> content/renderer/media/webrtc/video_destination_handler.cc >> File content/renderer/media/webrtc/video_destination_handler.cc (right): >> >> https://codereview.chromium.org/663183002/diff/60001/ >> content/renderer/media/webrtc/video_destination_handler.cc#newcode188 >> content/renderer/media/webrtc/video_destination_handler.cc:188: (endian >> == XXXA) ? libyuv::ARGBToI420 : libyuv::BGRAToI420; >> On 2014/10/22 16:56:39, thorcarpenter wrote: >> >>> can we do >>> >> >> xxxToI420 = (endian == XXXA || endian == UNKNOWN) ? libyuv::ARGBToI420 >>> >> : >> >>> libyuv::BGRAToI420; >>> >> >> Since ARGBToI420 is the future stable version of this code, and it's >>> >> possible >> >>> for this code to make a mistake in the case of a pure blue frame, >>> >> let's error on >> >>> the side of long term caution. >>> >> >> Done. >> >> https://codereview.chromium.org/663183002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm On Wed, Oct 22, 2014 at 10:56 AM, Thor Carpenter <thorcarpenter@google.com> wrote: > oh, after rereading, it looks right. staring at too many endians is > confusing :-) > > On Wed, Oct 22, 2014 at 10:47 AM, Thor Carpenter <thorcarpenter@google.com > > wrote: > >> perhaps gcl upload? >> >> I'm still seeing (endian_ == AXXX) >> >> On Wed, Oct 22, 2014 at 10:27 AM, <magjed@chromium.org> wrote: >> >>> PTAL. I changed so that the endian is stored as a member variable. If we >>> can >>> figure it out once, we won't do any more checks. >>> >>> >>> https://codereview.chromium.org/663183002/diff/20001/ >>> content/renderer/media/webrtc/video_destination_handler.cc >>> File content/renderer/media/webrtc/video_destination_handler.cc (right): >>> >>> https://codereview.chromium.org/663183002/diff/20001/ >>> content/renderer/media/webrtc/video_destination_handler.cc#newcode158 >>> content/renderer/media/webrtc/video_destination_handler.cc:158: // Due >>> to confusion about endianness, we try to determine it from the data. >>> On 2014/10/21 21:14:48, thorcarpenter wrote: >>> >>>> I like this, but consider adding TODO and referencing bug for when we >>>> >>> can remove >>> >>>> this and always do ARGBToI420. Probably m41 time frame. >>>> >>> >>> Done. >>> >>> https://codereview.chromium.org/663183002/diff/60001/ >>> content/renderer/media/webrtc/video_destination_handler.cc >>> File content/renderer/media/webrtc/video_destination_handler.cc (right): >>> >>> https://codereview.chromium.org/663183002/diff/60001/ >>> content/renderer/media/webrtc/video_destination_handler.cc#newcode188 >>> content/renderer/media/webrtc/video_destination_handler.cc:188: (endian >>> == XXXA) ? libyuv::ARGBToI420 : libyuv::BGRAToI420; >>> On 2014/10/22 16:56:39, thorcarpenter wrote: >>> >>>> can we do >>>> >>> >>> xxxToI420 = (endian == XXXA || endian == UNKNOWN) ? libyuv::ARGBToI420 >>>> >>> : >>> >>>> libyuv::BGRAToI420; >>>> >>> >>> Since ARGBToI420 is the future stable version of this code, and it's >>>> >>> possible >>> >>>> for this code to make a mistake in the case of a pure blue frame, >>>> >>> let's error on >>> >>>> the side of long term caution. >>>> >>> >>> Done. >>> >>> https://codereview.chromium.org/663183002/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yeah, sorry, I should have been more clear. I swapped XXXA to AXXX instead of the way you suggested. On Wed, Oct 22, 2014 at 7:56 PM, Thor Carpenter <thorcarpenter@google.com> wrote: > oh, after rereading, it looks right. staring at too many endians is > confusing :-) > > On Wed, Oct 22, 2014 at 10:47 AM, Thor Carpenter <thorcarpenter@google.com > > wrote: > >> perhaps gcl upload? >> >> I'm still seeing (endian_ == AXXX) >> >> On Wed, Oct 22, 2014 at 10:27 AM, <magjed@chromium.org> wrote: >> >>> PTAL. I changed so that the endian is stored as a member variable. If we >>> can >>> figure it out once, we won't do any more checks. >>> >>> >>> https://codereview.chromium.org/663183002/diff/20001/ >>> content/renderer/media/webrtc/video_destination_handler.cc >>> File content/renderer/media/webrtc/video_destination_handler.cc (right): >>> >>> https://codereview.chromium.org/663183002/diff/20001/ >>> content/renderer/media/webrtc/video_destination_handler.cc#newcode158 >>> content/renderer/media/webrtc/video_destination_handler.cc:158: // Due >>> to confusion about endianness, we try to determine it from the data. >>> On 2014/10/21 21:14:48, thorcarpenter wrote: >>> >>>> I like this, but consider adding TODO and referencing bug for when we >>>> >>> can remove >>> >>>> this and always do ARGBToI420. Probably m41 time frame. >>>> >>> >>> Done. >>> >>> https://codereview.chromium.org/663183002/diff/60001/ >>> content/renderer/media/webrtc/video_destination_handler.cc >>> File content/renderer/media/webrtc/video_destination_handler.cc (right): >>> >>> https://codereview.chromium.org/663183002/diff/60001/ >>> content/renderer/media/webrtc/video_destination_handler.cc#newcode188 >>> content/renderer/media/webrtc/video_destination_handler.cc:188: (endian >>> == XXXA) ? libyuv::ARGBToI420 : libyuv::BGRAToI420; >>> On 2014/10/22 16:56:39, thorcarpenter wrote: >>> >>>> can we do >>>> >>> >>> xxxToI420 = (endian == XXXA || endian == UNKNOWN) ? libyuv::ARGBToI420 >>>> >>> : >>> >>>> libyuv::BGRAToI420; >>>> >>> >>> Since ARGBToI420 is the future stable version of this code, and it's >>>> >>> possible >>> >>>> for this code to make a mistake in the case of a pure blue frame, >>>> >>> let's error on >>> >>>> the side of long term caution. >>>> >>> >>> Done. >>> >>> https://codereview.chromium.org/663183002/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
What is the reason why image_data->format() == PP_IMAGEDATAFORMAT_BGRA_PREMUL can't be used to specify/detect the byte ordering explicitly rather than using a heuristic? https://codereview.chromium.org/663183002/diff/80001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/663183002/diff/80001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:191: (endian_ == AXXX) ? libyuv::BGRAToI420 : libyuv::ARGBToI420; This reads very strangely. If endian_ is AXXX then use the XXXA routine? Doesn't this mean that the meanings of AXXX and XXXA are reversed? In particular, looking at the code that sets endian_, XXXA is used when the in-memory byte-order is AXXX, which seems odd.
brucedawson: Regarding image_data->format(). We always expect PP_IMAGEDATAFORMAT_BGRA_PREMUL, that DCHECK will crash otherwise in debug. The confusion is about what PP_IMAGEDATAFORMAT_BGRA_PREMUL actually stands for. The heuristics determines if it is BGRA or ARGB. https://codereview.chromium.org/663183002/diff/80001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/663183002/diff/80001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:191: (endian_ == AXXX) ? libyuv::BGRAToI420 : libyuv::ARGBToI420; On 2014/10/22 17:58:41, brucedawson wrote: > This reads very strangely. If endian_ is AXXX then use the XXXA routine? Doesn't > this mean that the meanings of AXXX and XXXA are reversed? > > In particular, looking at the code that sets endian_, XXXA is used when the > in-memory byte-order is AXXX, which seems odd. I agree that it looks strange, but AXXX/XXXA is in memory byte order. For example, if row_ptr[x * 4 + 0] != 255 then the first byte is _not_ Alpha, so it must be XXXA. The discrepancy between memory byte order and libyuv naming is probably the source of this confusion to start with.
Would it not be sensible to pass in a new IMAGEDATAFORMAT that specifies the new interpretation of PP_IMAGEDATAFORMAT_BGRA_PREMUL, rather than scanning the image data? Something like PP_IMAGEDATAFORMAT_BGRA_MEMORYORDER_PREMUL or whatever makes sense. The detection code makes sense now. I should have understood that. Regarding AXXX versus XXXA, are we setting ourselves up for future problems by intermingling memory-order definitions with libyuv/fourcc definitions? Even though I don't like the libyuv/fourcc definitions it seems problematic to use a reversed naming convention. Maybe at least comment it somewhere, either way, to avoid future confusion. On Wed, Oct 22, 2014 at 11:18 AM, <magjed@chromium.org> wrote: > brucedawson: Regarding image_data->format(). We always expect > PP_IMAGEDATAFORMAT_BGRA_PREMUL, that DCHECK will crash otherwise in > debug. The > confusion is about what PP_IMAGEDATAFORMAT_BGRA_PREMUL actually stands > for. The > heuristics determines if it is BGRA or ARGB. > > > https://codereview.chromium.org/663183002/diff/80001/ > content/renderer/media/webrtc/video_destination_handler.cc > File content/renderer/media/webrtc/video_destination_handler.cc (right): > > https://codereview.chromium.org/663183002/diff/80001/ > content/renderer/media/webrtc/video_destination_handler.cc#newcode191 > content/renderer/media/webrtc/video_destination_handler.cc:191: (endian_ > == AXXX) ? libyuv::BGRAToI420 : libyuv::ARGBToI420; > On 2014/10/22 17:58:41, brucedawson wrote: > >> This reads very strangely. If endian_ is AXXX then use the XXXA >> > routine? Doesn't > >> this mean that the meanings of AXXX and XXXA are reversed? >> > > In particular, looking at the code that sets endian_, XXXA is used >> > when the > >> in-memory byte-order is AXXX, which seems odd. >> > > I agree that it looks strange, but AXXX/XXXA is in memory byte order. > For example, if > row_ptr[x * 4 + 0] != 255 > then the first byte is _not_ Alpha, so it must be XXXA. The discrepancy > between memory byte order and libyuv naming is probably the source of > this confusion to start with. > > https://codereview.chromium.org/663183002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #5 (id:100001) has been deleted
brucedawson: I added some comments to clarify the code. Regarding sending in a new format that specifies interpretation: This is just an intermediate fix, all this code will be removed again when both Chromium and Pepper have changed the endian, so I don't think it's worth the trouble to add more signaling.
Apologies if this is a dumb question, but how do we guarantee that somebody doesn't stay on an old version of Chrome indefinitely. That is, if "all this code will be removed again when both Chromium and Pepper have changed the endian" when will it ever be 100% safe to remove this code. I would expect that a new IMAGEDATAFORMAT would let us know dynamically when to use the new path.
Yes, there will always be some small percent of users that are on really old chrome and a different percent that are on really old plugins. At some point we'll remove the backwards compatibility and just break those few users. On Wed, Oct 22, 2014 at 1:05 PM, <brucedawson@chromium.org> wrote: > Apologies if this is a dumb question, but how do we guarantee that somebody > doesn't stay on an old version of Chrome indefinitely. That is, if "all > this > code will be removed again when both Chromium and Pepper have changed the > endian" when will it ever be 100% safe to remove this code. I would expect > that > a new IMAGEDATAFORMAT would let us know dynamically when to use the new > path. > > https://codereview.chromium.org/663183002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... content/renderer/media/webrtc/video_destination_handler.cc:169: // Due to confusion about endianness, we try to determine it from the data. I'm not aware of any 'confusion'. Pepper originally only supported BGRA - big endian, A,R,G,B. Now that we can use ARGB, we're switching. But we dont have an easy way to confirm the chrome version, so we're auto detecting the pixel channel order. https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... content/renderer/media/webrtc/video_destination_handler.cc:174: for (int y = 0; y < height && endian_ == UNKNOWN; ++y) { consider making endian_ static so it detects once. https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... content/renderer/media/webrtc/video_destination_handler.cc:191: // libyuv uses reversed memory byte order, that is why the naming is reversed. comment misleading. libyuv specifies fourcc/channel ordering the same as webrtc. FYI Chrome supports 4 channel orderings: ARGB, BGRA, RGBA, and ABGR. https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... content/renderer/media/webrtc/video_destination_handler.cc:194: xxxxToI420(data, consider using ConvertToI420, which takes the fourcc as a parameter. is 'auto' safe to use?
fbarchard: PTAL. https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... content/renderer/media/webrtc/video_destination_handler.cc:169: // Due to confusion about endianness, we try to determine it from the data. On 2014/10/22 21:47:43, fbarchard wrote: > I'm not aware of any 'confusion'. Pepper originally only supported BGRA - big > endian, A,R,G,B. Now that we can use ARGB, we're switching. But we dont have > an easy way to confirm the chrome version, so we're auto detecting the pixel > channel order. I removed 'confusion' from the comment. Something is inconsistent in the current code though, because the comments for PP_ImageDataFormat state: * The third part of each enumeration value describes the memory layout from * the lowest address to the highest. For example, BGRA means the B component * is stored in the lowest address, no matter what endianness the platform is * using. and this is currently not true. Link to PP_ImageDataFormat: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/ppb_image_... https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... content/renderer/media/webrtc/video_destination_handler.cc:174: for (int y = 0; y < height && endian_ == UNKNOWN; ++y) { On 2014/10/22 21:47:43, fbarchard wrote: > consider making endian_ static so it detects once. I prefer to keep it per stream. static variables for POD is allowed, but not encouraged. We recently moved the color conversion to the IO thread in this CL: https://codereview.chromium.org/631903003/ so it should be thread safe now, but we might revert that CL in the future. Also, making it static is a minor optimization. https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... content/renderer/media/webrtc/video_destination_handler.cc:191: // libyuv uses reversed memory byte order, that is why the naming is reversed. On 2014/10/22 21:47:43, fbarchard wrote: > comment misleading. libyuv specifies fourcc/channel ordering the same as > webrtc. > FYI Chrome supports 4 channel orderings: ARGB, BGRA, RGBA, and ABGR. Done. https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... content/renderer/media/webrtc/video_destination_handler.cc:194: xxxxToI420(data, On 2014/10/22 21:47:43, fbarchard wrote: > consider using ConvertToI420, which takes the fourcc as a parameter. > > is 'auto' safe to use? I can't pass the src stride to ConvertToI420 - it assumes it is tightly packed. 'auto' was recently approved for chromium, ref: http://chromium-cpp.appspot.com/
On 2014/10/23 10:59:36, magjed wrote: > fbarchard: PTAL. > > https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... > File content/renderer/media/webrtc/video_destination_handler.cc (right): > > https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... > content/renderer/media/webrtc/video_destination_handler.cc:169: // Due to > confusion about endianness, we try to determine it from the data. > On 2014/10/22 21:47:43, fbarchard wrote: > > I'm not aware of any 'confusion'. Pepper originally only supported BGRA - big > > endian, A,R,G,B. Now that we can use ARGB, we're switching. But we dont > have > > an easy way to confirm the chrome version, so we're auto detecting the pixel > > channel order. > > I removed 'confusion' from the comment. Something is inconsistent in the current > code though, because the comments for PP_ImageDataFormat state: > * The third part of each enumeration value describes the memory layout from > * the lowest address to the highest. For example, BGRA means the B component > * is stored in the lowest address, no matter what endianness the platform is > * using. > and this is currently not true. Link to PP_ImageDataFormat: > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/ppb_image_... > > https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... > content/renderer/media/webrtc/video_destination_handler.cc:174: for (int y = 0; > y < height && endian_ == UNKNOWN; ++y) { > On 2014/10/22 21:47:43, fbarchard wrote: > > consider making endian_ static so it detects once. > > I prefer to keep it per stream. static variables for POD is allowed, but not > encouraged. We recently moved the color conversion to the IO thread in this CL: > https://codereview.chromium.org/631903003/ > so it should be thread safe now, but we might revert that CL in the future. > Also, making it static is a minor optimization. > > https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... > content/renderer/media/webrtc/video_destination_handler.cc:191: // libyuv uses > reversed memory byte order, that is why the naming is reversed. > On 2014/10/22 21:47:43, fbarchard wrote: > > comment misleading. libyuv specifies fourcc/channel ordering the same as > > webrtc. > > FYI Chrome supports 4 channel orderings: ARGB, BGRA, RGBA, and ABGR. > > Done. > > https://codereview.chromium.org/663183002/diff/120001/content/renderer/media/... > content/renderer/media/webrtc/video_destination_handler.cc:194: xxxxToI420(data, > On 2014/10/22 21:47:43, fbarchard wrote: > > consider using ConvertToI420, which takes the fourcc as a parameter. > > > > is 'auto' safe to use? > > I can't pass the src stride to ConvertToI420 - it assumes it is tightly packed. > 'auto' was recently approved for chromium, ref: http://chromium-cpp.appspot.com/ Making endian_ static is tempting because of the performance improvement in degenerate cases, but it would also raise the risk of getting stuck in a bad state if the first frame was a pathological one. That alone seems like enough reason to keep it non-static.
magjed@chromium.org changed reviewers: + perkj@chromium.org
perkj: Can you take a look as well?
On 2014/10/23 20:46:43, magjed wrote: > perkj: Can you take a look as well? Well - it is temporary right? lgtm
The CQ bit was checked by magjed@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663183002/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8cd330dced386cf0f3eb6888af8b9f46b90d8d1c Cr-Commit-Position: refs/heads/master@{#301079}
Message was sent while issue was closed.
I'm implementing the corresponding change in the nacl effects plugin and I think the AXXX logic here is backwards. libyuv says ARGB means src[0]:B src[1]:G src[2]:R src[3]:A (see here: https://cs.corp.google.com/#piper///depot/google3/third_party/libyuv/files/so... ) In this change we're saying essentially if px_ptr[3] is not A then AXXX, but in libyuv px_ptr[3] is A for AXXX... Do we have a canary build with this change yet to verify? An ML hangout should look pretty broken if I'm not mistaken. On Fri, Oct 24, 2014 at 2:37 AM, <commit-bot@chromium.org> wrote: > Patchset 6 (id:??) landed as > https://crrev.com/8cd330dced386cf0f3eb6888af8b9f46b90d8d1c > Cr-Commit-Position: refs/heads/master@{#301079} > > https://codereview.chromium.org/663183002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2014/10/24 16:35:21, chromium-reviews wrote: > I'm implementing the corresponding change in the nacl effects plugin and I > think the AXXX logic here is backwards. > > libyuv says ARGB means src[0]:B src[1]:G src[2]:R src[3]:A (see here: > https://cs.corp.google.com/#piper///depot/google3/third_party/libyuv/files/so... > ) > > In this change we're saying essentially if px_ptr[3] is not A then AXXX, > but in libyuv px_ptr[3] is A for AXXX... > > Do we have a canary build with this change yet to verify? An ML hangout > should look pretty broken if I'm not mistaken. > > On Fri, Oct 24, 2014 at 2:37 AM, <mailto:commit-bot@chromium.org> wrote: > > > Patchset 6 (id:??) landed as > > https://crrev.com/8cd330dced386cf0f3eb6888af8b9f46b90d8d1c > > Cr-Commit-Position: refs/heads/master@{#301079} > > > > https://codereview.chromium.org/663183002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. No, it should be correct. I'm using the same naming convention for AXXX as PP_ImageDataFormat does, which is in memory order. As you say, libyuv uses: ARGB: src[0]:B src[1]:G src[2]:R src[3]:A which is in the reversed order of that. There are comments in the code explaining the difference.
Message was sent while issue was closed.
ok yeah you're right. I missed where you were actually selecting the right libyuv::XXXXtoI420 version. On Fri, Oct 24, 2014 at 9:58 AM, <magjed@chromium.org> wrote: > On 2014/10/24 16:35:21, chromium-reviews wrote: > >> I'm implementing the corresponding change in the nacl effects plugin and I >> think the AXXX logic here is backwards. >> > > libyuv says ARGB means src[0]:B src[1]:G src[2]:R src[3]:A (see here: >> > > https://cs.corp.google.com/#piper///depot/google3/third_ > party/libyuv/files/source/row_common.cc&rcl=78325477&l=1995 > >> ) >> > > In this change we're saying essentially if px_ptr[3] is not A then AXXX, >> but in libyuv px_ptr[3] is A for AXXX... >> > > Do we have a canary build with this change yet to verify? An ML hangout >> should look pretty broken if I'm not mistaken. >> > > On Fri, Oct 24, 2014 at 2:37 AM, <mailto:commit-bot@chromium.org> wrote: >> > > > Patchset 6 (id:??) landed as >> > https://crrev.com/8cd330dced386cf0f3eb6888af8b9f46b90d8d1c >> > Cr-Commit-Position: refs/heads/master@{#301079} >> > >> > https://codereview.chromium.org/663183002/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > No, it should be correct. I'm using the same naming convention for AXXX as > PP_ImageDataFormat does, which is in memory order. As you say, libyuv uses: > ARGB: src[0]:B src[1]:G src[2]:R src[3]:A > which is in the reversed order of that. There are comments in the code > explaining the difference. > > > > > > https://codereview.chromium.org/663183002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
