Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
950633003:
PDF: remove last use of SkPDFImage (Closed)
|
Created:
5 years, 10 months ago by hal.canary Modified:
5 years, 9 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionPDF: remove last use of SkPDFImage
Add a GM.
BUG=skia:255
Committed: https://skia.googlesource.com/skia/+/db0dcc7436375e5d59c27f9011f09b64de407c9d
Patch Set 1 #Patch Set 2 : 2015-02-21 (Saturday) 10:55:02 EST #
Total comments: 30
Patch Set 3 : 2015-03-19 (Thursday) 21:02:14 EDT #
Total comments: 8
Patch Set 4 : comments from reed@ - 2015-03-20 (Friday) 09:49:34 EDT #
Total comments: 8
Patch Set 5 : comments from reed@ again - 2015-03-20 (Friday) 11:25:42 EDT #Patch Set 6 : generate an indexed bitmap - 2015-03-20 (Friday) 14:33:08 EDT #Patch Set 7 : 2015-03-20 (Friday) 15:07:42 EDT #Patch Set 8 : 2015-03-20 (Friday) 15:22:20 EDT #
Messages
Total messages: 34 (13 generated)
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950633003/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-02-21 21:51 UTC
halcanary@google.com changed reviewers: + mtklein@google.com
ptal. There are fifty ways to skin the cat when it comes to factoring the code in SkPDFBitmap. 358 insertions, 889 deletions.
That's an awful lot of color type switches and careful code. What do you think would be the perf impact if we just kept a fast path for N32 and used SkBitmap::getColor() for the rest? Or even, SkBitmap::getColor() across the board? If that's stupidly slow, I wonder if some sort of compromise like a scanline-by-scanline SkBitmap::readPixels() would be just as fast and simpler to manage. https://codereview.chromium.org/950633003/diff/20001/gm/all_bitmap_configs.cpp File gm/all_bitmap_configs.cpp (right): https://codereview.chromium.org/950633003/diff/20001/gm/all_bitmap_configs.cp... gm/all_bitmap_configs.cpp:28: SkAutoTUnref<SkSurface> surf565(SkSurface::NewRaster(SkImageInfo::Make( Why not use SkBitmap::copyTo() here too? https://codereview.chromium.org/950633003/diff/20001/gm/all_bitmap_configs.cp... gm/all_bitmap_configs.cpp:58: uint8_t spectrum[256]; Seems fine, but we really only need 128, right? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp File src/pdf/SkPDFBitmap.cpp (right): https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:42: static void to_rgb24(SkPMColor color, U8CPU alpha, uint8_t* rgb) { This seems weird. Why do we pass alpha twice? It's always color's alpha, right? Might be nice to pass rgb as uint8_t rgb[3]? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:49: static void to_rgb24(SkPMColor color, uint8_t* rgb) { This is for the alpha == 0xFF case? Let's at least assert it, maybe merge this into the other to_rgb24? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:62: SkASSERT(false); // We only use this to sample around transparent pixels, so only 4444 and 8888 make sense. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:81: uint8_t n = 0; This seems... unnecessarily precise? unsigned seems fine for n too, or really, int? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:83: int yrange[2] = {SkTMax(yOrig - 1, 0), SkTMin(bm.height(), yOrig + 2)}; Might want to swap the orders of the arguments to max and min so they look like a clamp SkTMax(0, yOrig-1), SkTMin(yOrig+2, bm.height()) Might also want to use <= so you can write -1, +1 ? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:86: for (int x = xrange[0]; x < xrange[1]; ++x) { Seems like when there are a lot of transparent pixels, this will examine each pixel many times more than if we did a pass over the whole image at once. I guess this is still dwarfed by deflate in CPU time? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:172: } // not actually rgb24 format. Let's explain what it is rather than what it's not? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:208: fill_stream(out, '\x00', pixel_count(bm)); It seems like it's worth promoting this up to an assertion. Index8 doesn't make much sense without a color table, right? I might just use SkBitmap::getIndex8Color()? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:352: // Max size as a string is 1.5k. Some sort of assert here then? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:399: static bool is_transparent_pixels(const SkBitmap& bm) { pixels_are_transparent? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:406: alpha |= SkGetPackedA32(*src++); Why don't we just bail out the first time we see a non-zero alpha? Why wait until the end of the scanline? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:453: alpha |= SkGetPackedA32((*ct)[*src++]); Same question? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:505: if (is_transparent(bm)) { Just curious, do we really see fully transparent bitmaps? Is this check really worth the bother and ~70 extra lines of code? All the normal paths will work just fine, right? And presumably all-zero bitmaps will compress very well.
Patchset #3 (id:40001) has been deleted
On 2015/02/23 14:26:53, mtklein wrote: > That's an awful lot of color type switches and careful code. > > What do you think would be the perf impact if we just kept a fast path for N32 > and used SkBitmap::getColor() for the rest? Or even, SkBitmap::getColor() > across the board? If that's stupidly slow, I wonder if some sort of compromise > like a scanline-by-scanline SkBitmap::readPixels() would be just as fast and > simpler to manage. I don't worry too much about perf, but SkBitmap::readPixels doesn't seem like the right thing to do, because of the alpha scaling problem I've tried to simplify this as much as I can (e.g. removing 4444 support since that's not used much anywhere). Derek wants 565 to be fast and well supported. Indexed color works best if the PDF keeps the index. I've tried to clean up the codepaths as much as poassible.
https://codereview.chromium.org/950633003/diff/20001/gm/all_bitmap_configs.cpp File gm/all_bitmap_configs.cpp (right): https://codereview.chromium.org/950633003/diff/20001/gm/all_bitmap_configs.cp... gm/all_bitmap_configs.cpp:28: SkAutoTUnref<SkSurface> surf565(SkSurface::NewRaster(SkImageInfo::Make( On 2015/02/23 14:26:52, mtklein wrote: > Why not use SkBitmap::copyTo() here too? Done. https://codereview.chromium.org/950633003/diff/20001/gm/all_bitmap_configs.cp... gm/all_bitmap_configs.cpp:58: uint8_t spectrum[256]; On 2015/02/23 14:26:52, mtklein wrote: > Seems fine, but we really only need 128, right? I'm shifting it over one pixel for each scanline. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp File src/pdf/SkPDFBitmap.cpp (right): https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:42: static void to_rgb24(SkPMColor color, U8CPU alpha, uint8_t* rgb) { On 2015/02/23 14:26:52, mtklein wrote: > This seems weird. Why do we pass alpha twice? It's always color's alpha, > right? I didn't want to recalculate it, but that was too much optimization. Fixed. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:49: static void to_rgb24(SkPMColor color, uint8_t* rgb) { On 2015/02/23 14:26:52, mtklein wrote: > This is for the alpha == 0xFF case? Let's at least assert it, maybe merge this > into the other to_rgb24? this is removed. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:62: SkASSERT(false); On 2015/02/23 14:26:52, mtklein wrote: > // We only use this to sample around transparent pixels, so only 4444 and 8888 > make sense. Done. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:81: uint8_t n = 0; On 2015/02/23 14:26:53, mtklein wrote: > This seems... unnecessarily precise? unsigned seems fine for n too, or really, > int? Done. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:83: int yrange[2] = {SkTMax(yOrig - 1, 0), SkTMin(bm.height(), yOrig + 2)}; On 2015/02/23 14:26:53, mtklein wrote: > Might want to swap the orders of the arguments to max and min so they look like > a clamp > > SkTMax(0, yOrig-1), SkTMin(yOrig+2, bm.height()) > > Might also want to use <= so you can write -1, +1 ? <= is unexpected in C. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:86: for (int x = xrange[0]; x < xrange[1]; ++x) { On 2015/02/23 14:26:53, mtklein wrote: > Seems like when there are a lot of transparent pixels, this will examine each > pixel many times more than if we did a pass over the whole image at once. I > guess this is still dwarfed by deflate in CPU time? I thinks so. I tried to think of how to do this in one pass, (think of doing a blur by scattering rather than gathering), but it doesn't seem worth the headache to make that change. Also, I changed the algorithm a little, to weight neighbors by opacity. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:172: } // not actually rgb24 format. On 2015/02/23 14:26:53, mtklein wrote: > Let's explain what it is rather than what it's not? Done. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:208: fill_stream(out, '\x00', pixel_count(bm)); On 2015/02/23 14:26:53, mtklein wrote: > It seems like it's worth promoting this up to an assertion. Index8 doesn't make > much sense without a color table, right? I might just use > SkBitmap::getIndex8Color()? Done. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:352: // Max size as a string is 1.5k. On 2015/02/23 14:26:53, mtklein wrote: > Some sort of assert here then? Done. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:399: static bool is_transparent_pixels(const SkBitmap& bm) { On 2015/02/23 14:26:52, mtklein wrote: > pixels_are_transparent? I've stoped this check. Why lock the bitmap unnecessarily? https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:406: alpha |= SkGetPackedA32(*src++); On 2015/02/23 14:26:53, mtklein wrote: > Why don't we just bail out the first time we see a non-zero alpha? Why wait > until the end of the scanline? Acknowledged. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:453: alpha |= SkGetPackedA32((*ct)[*src++]); On 2015/02/23 14:26:52, mtklein wrote: > Same question? Acknowledged. https://codereview.chromium.org/950633003/diff/20001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:505: if (is_transparent(bm)) { On 2015/02/23 14:26:53, mtklein wrote: > Just curious, do we really see fully transparent bitmaps? Is this check really > worth the bother and ~70 extra lines of code? All the normal paths will work > just fine, right? And presumably all-zero bitmaps will compress very well. Done.
Patchset #3 (id:60001) has been deleted
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp File src/pdf/SkPDFBitmap.cpp (right): https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:66: for (int y = yrange[0]; y < yrange[1]; ++y) { range[0], range[1]... pretty hard to read. Why not ymin and ymax? https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:77: rgb[0] = 255 * r / a; SkToU8(...) -- this guy asserts in debug that we're not overflowing a byte, and adds a cast to silence possible warnings. https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:99: static void bitmap_to_rgb24(const SkBitmap& bitmap, SkWStream* out) { Since this sometimes writes index8 or gray8, perhaps the function should have a different name? https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:206: case kARGB_4444_SkColorType: // converted to N32 what does "converted to N32" mean? Does it mean we should never see this enum in this function? Should we assert that?
https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp File src/pdf/SkPDFBitmap.cpp (right): https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:66: for (int y = yrange[0]; y < yrange[1]; ++y) { On 2015/03/20 12:42:04, reed1 wrote: > range[0], range[1]... pretty hard to read. Why not ymin and ymax? Done. https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:77: rgb[0] = 255 * r / a; On 2015/03/20 12:42:03, reed1 wrote: > SkToU8(...) -- this guy asserts in debug that we're not overflowing a byte, and > adds a cast to silence possible warnings. Done. https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:99: static void bitmap_to_rgb24(const SkBitmap& bitmap, SkWStream* out) { On 2015/03/20 12:42:03, reed1 wrote: > Since this sometimes writes index8 or gray8, perhaps the function should have a > different name? Done. https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... src/pdf/SkPDFBitmap.cpp:206: case kARGB_4444_SkColorType: // converted to N32 On 2015/03/20 12:42:03, reed1 wrote: > what does "converted to N32" mean? Does it mean we should never see > this enum in this function? Should we assert that? That's why I skassert(false).
On 2015/03/20 13:49:51, Hal Canary wrote: > https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp > File src/pdf/SkPDFBitmap.cpp (right): > > https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... > src/pdf/SkPDFBitmap.cpp:66: for (int y = yrange[0]; y < yrange[1]; ++y) { > On 2015/03/20 12:42:04, reed1 wrote: > > range[0], range[1]... pretty hard to read. Why not ymin and ymax? > > Done. > > https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... > src/pdf/SkPDFBitmap.cpp:77: rgb[0] = 255 * r / a; > On 2015/03/20 12:42:03, reed1 wrote: > > SkToU8(...) -- this guy asserts in debug that we're not overflowing a byte, > and > > adds a cast to silence possible warnings. > > Done. > > https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... > src/pdf/SkPDFBitmap.cpp:99: static void bitmap_to_rgb24(const SkBitmap& bitmap, > SkWStream* out) { > On 2015/03/20 12:42:03, reed1 wrote: > > Since this sometimes writes index8 or gray8, perhaps the function should have > a > > different name? > > Done. > > https://codereview.chromium.org/950633003/diff/80001/src/pdf/SkPDFBitmap.cpp#... > src/pdf/SkPDFBitmap.cpp:206: case kARGB_4444_SkColorType: // converted to N32 > On 2015/03/20 12:42:03, reed1 wrote: > > what does "converted to N32" mean? Does it mean we should never see > > this enum in this function? Should we assert that? > > That's why I skassert(false). Doh!
https://codereview.chromium.org/950633003/diff/100001/src/pdf/SkPDFBitmap.cpp File src/pdf/SkPDFBitmap.cpp (right): https://codereview.chromium.org/950633003/diff/100001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:102: static size_t pdf_pixel_width(SkColorType ct) { nit: your comment is very clear, and also a bit different then the actual name of the function! How about renaming the function so that the comment is unnecessary? e.g. count_color_components_for_pdf(ct) https://codereview.chromium.org/950633003/diff/100001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:318: result->appendInt(table->count() - 1); can a colortable have 0 entries? https://codereview.chromium.org/950633003/diff/100001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:322: char tableArray[768]; tableArray[3 * 256] or some other way to document the literal 768? https://codereview.chromium.org/950633003/diff/100001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:323: SkASSERT(3u * table->count() <= sizeof(tableArray)); I know its bytes, but I think the assert would be clearer if we compared counts to counts... i.e. SK_ARRAY_COUNT(tableArray)
https://codereview.chromium.org/950633003/diff/100001/src/pdf/SkPDFBitmap.cpp File src/pdf/SkPDFBitmap.cpp (right): https://codereview.chromium.org/950633003/diff/100001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:102: static size_t pdf_pixel_width(SkColorType ct) { On 2015/03/20 14:15:49, reed1 wrote: > nit: your comment is very clear, and also a bit different then the actual name > of the function! How about renaming the function so that the comment is > unnecessary? > > e.g. > > count_color_components_for_pdf(ct) Done. https://codereview.chromium.org/950633003/diff/100001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:318: result->appendInt(table->count() - 1); On 2015/03/20 14:15:49, reed1 wrote: > can a colortable have 0 entries? Apparently so. I don't know if that is ever valid, but I'll allow for that. https://codereview.chromium.org/950633003/diff/100001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:322: char tableArray[768]; On 2015/03/20 14:15:49, reed1 wrote: > tableArray[3 * 256] or some other way to document the literal 768? Done. https://codereview.chromium.org/950633003/diff/100001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:323: SkASSERT(3u * table->count() <= sizeof(tableArray)); On 2015/03/20 14:15:49, reed1 wrote: > I know its bytes, but I think the assert would be clearer if we compared counts > to counts... i.e. SK_ARRAY_COUNT(tableArray) Done.
I have run out of nits. lgtm
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950633003/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://skia.googlesource.com/skia/+/86ad8d643624a55b02e529100bbe4e2940115fa1
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:120001) has been created in https://codereview.chromium.org/1024113002/ by reed@google.com. The reason for reverting is: static void draw(SkCanvas* canvas, const SkPaint& p, const SkBitmap& src, SkColorType colorType, const char text[]) { SkASSERT(src.colorType() == colorType); canvas->drawBitmap(src, 0.0f, 0.0f); canvas->drawText(text, strlen(text), 0.0f, 12.0f, p); } This assert is firing, at least on macs, where all images get decoded into 32bit at the moment..
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:120001) has been created in https://codereview.chromium.org/1023223002/ by scroggo@google.com. The reason for reverting is: Broke on Mac due to line 82 in the test. We assert that a gif is Index8, but Mac's decoder decodes it to 8888 (Not sure what windows does...).
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/950633003/#ps140001 (title: "generate an indexed bitmap - 2015-03-20 (Friday) 14:33:08 EDT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950633003/140001
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/950633003/#ps160001 (title: "2015-03-20 (Friday) 15:07:42 EDT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950633003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/950633003/#ps180001 (title: "2015-03-20 (Friday) 15:22:20 EDT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950633003/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://skia.googlesource.com/skia/+/db0dcc7436375e5d59c27f9011f09b64de407c9d |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
