Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1291933002:
File video capture device supports MJPEG format (Closed)
DescriptionFile video capture device supports MJPEG format
Because input file can be a MJPEG stream video which concatenates many
MJPEG/JPEG images directly, jpeg parser should support to detect EOI
marker.
BUG=517303
TEST=test on peach_pi and squawks with Y4M and JPEG files. Make sure jpeg
histogram is correct while playing JPEG file.
Committed: https://crrev.com/cc92a03c8029eda1e443d7ecd1750976d6aa8707
Cr-Commit-Position: refs/heads/master@{#344749}
Patch Set 1 #
Total comments: 22
Patch Set 2 : address kcwu@ #
Total comments: 6
Patch Set 3 : address kcwu@ #
Total comments: 6
Patch Set 4 : #
Total comments: 4
Patch Set 5 : fix nits #
Total comments: 2
Patch Set 6 : address wucheng@ #
Total comments: 15
Patch Set 7 : address wucheng@ #
Total comments: 26
Patch Set 8 : refactor #
Total comments: 34
Patch Set 9 : address all comments #Patch Set 10 : fix performance issue #
Total comments: 5
Patch Set 11 : use memchr #
Total comments: 2
Patch Set 12 : fix nits #
Total comments: 8
Patch Set 13 : fix nits #Patch Set 14 : fix windows compile error #Patch Set 15 : fix windows compile error #
Messages
Total messages: 54 (14 generated)
henryhsu@chromium.org changed reviewers: + kcwu@chromium.org, posciak@chromium.org, wuchengli@chromium.org
PTAL
https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... media/capture/video/file_video_capture_device.cc:141: LOG(ERROR) << "File memory map error: " << file_path.value(); don't we need to return nullptr here? https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... media/capture/video/file_video_capture_device.cc:230: int image_size = frame_size_ + kY4MSimpleFrameDelimiterSize; please put Y4M value in else part of 'if'. https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... media/capture/video/file_video_capture_device.cc:245: if (current_byte_index_ >= static_cast<int64_t>(mapped_file_->length())) how about change current_byte_index_ to size_t ? https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... File media/capture/video/file_video_capture_device.h (right): https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... media/capture/video/file_video_capture_device.h:34: // static remove this line? https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.cc File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.c... media/filters/jpeg_parser.cc:272: static bool ParseEOI(const char* buffer, s/ParseEOI/SearchEOI/ ? https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.c... media/filters/jpeg_parser.cc:292: case 0x00: add break; https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.c... media/filters/jpeg_parser.cc:318: if (reader.remaining() < size) { if (!reader.Skip(size)) { ... } https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.c... media/filters/jpeg_parser.cc:439: result->image_size = eoi_ptr - buffer + 2; const int kEoiSize = 2; ? https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.h File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.h... media/filters/jpeg_parser.h:33: JPEG_RST0 = 0xD0, // Restart s/R/r/ https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.h... media/filters/jpeg_parser.h:99: size_t image_size; add comment to explain data_size and image_size. They are a bit confusing from the name. https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.h... media/filters/jpeg_parser.h:99: size_t image_size; Please exam this new value in the unit test.
https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... media/capture/video/file_video_capture_device.cc:141: LOG(ERROR) << "File memory map error: " << file_path.value(); On 2015/08/14 09:01:37, kcwu wrote: > don't we need to return nullptr here? not need. caller can use mapped_file->IsValid to check https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... media/capture/video/file_video_capture_device.cc:230: int image_size = frame_size_ + kY4MSimpleFrameDelimiterSize; On 2015/08/14 09:01:37, kcwu wrote: > please put Y4M value in else part of 'if'. Done. https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... media/capture/video/file_video_capture_device.cc:245: if (current_byte_index_ >= static_cast<int64_t>(mapped_file_->length())) On 2015/08/14 09:01:37, kcwu wrote: > how about change current_byte_index_ to size_t ? Done. https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... File media/capture/video/file_video_capture_device.h (right): https://codereview.chromium.org/1291933002/diff/1/media/capture/video/file_vi... media/capture/video/file_video_capture_device.h:34: // static On 2015/08/14 09:01:37, kcwu wrote: > remove this line? Done. Thanks. https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.cc File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.c... media/filters/jpeg_parser.cc:272: static bool ParseEOI(const char* buffer, On 2015/08/14 09:01:37, kcwu wrote: > s/ParseEOI/SearchEOI/ ? Done. https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.c... media/filters/jpeg_parser.cc:292: case 0x00: On 2015/08/14 09:01:37, kcwu wrote: > add break; Done. https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.c... media/filters/jpeg_parser.cc:318: if (reader.remaining() < size) { On 2015/08/14 09:01:37, kcwu wrote: > if (!reader.Skip(size)) { > ... > } Done. https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.c... media/filters/jpeg_parser.cc:439: result->image_size = eoi_ptr - buffer + 2; On 2015/08/14 09:01:37, kcwu wrote: > const int kEoiSize = 2; ? no. This 2 is SOI size which is read in beginning. https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.h File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.h... media/filters/jpeg_parser.h:33: JPEG_RST0 = 0xD0, // Restart On 2015/08/14 09:01:38, kcwu wrote: > s/R/r/ Done. https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.h... media/filters/jpeg_parser.h:99: size_t image_size; On 2015/08/14 09:01:37, kcwu wrote: > add comment to explain data_size and image_size. They are a bit confusing from > the name. Done. https://codereview.chromium.org/1291933002/diff/1/media/filters/jpeg_parser.h... media/filters/jpeg_parser.h:99: size_t image_size; On 2015/08/14 09:01:38, kcwu wrote: > Please exam this new value in the unit test. Done.
https://codereview.chromium.org/1291933002/diff/20001/media/capture/video/fil... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/20001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:194: if (!mapped_file_->IsValid()) { OpenFileForRead() may return nullptr. https://codereview.chromium.org/1291933002/diff/20001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:243: } else UNREACHABLE https://codereview.chromium.org/1291933002/diff/20001/media/capture/video/fil... File media/capture/video/file_video_capture_device_factory.cc (right): https://codereview.chromium.org/1291933002/diff/20001/media/capture/video/fil... media/capture/video/file_video_capture_device_factory.cc:68: if (!mapped_file->IsValid()) OpenFileForRead() may return nullptr
https://codereview.chromium.org/1291933002/diff/20001/media/capture/video/fil... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/20001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:194: if (!mapped_file_->IsValid()) { On 2015/08/14 11:50:04, kcwu wrote: > OpenFileForRead() may return nullptr. Done. https://codereview.chromium.org/1291933002/diff/20001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:243: } On 2015/08/14 11:50:04, kcwu wrote: > else > UNREACHABLE Done. https://codereview.chromium.org/1291933002/diff/20001/media/capture/video/fil... File media/capture/video/file_video_capture_device_factory.cc (right): https://codereview.chromium.org/1291933002/diff/20001/media/capture/video/fil... media/capture/video/file_video_capture_device_factory.cc:68: if (!mapped_file->IsValid()) On 2015/08/14 11:50:04, kcwu wrote: > OpenFileForRead() may return nullptr Done.
https://codereview.chromium.org/1291933002/diff/40001/media/capture/video/fil... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/40001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:192: DCHECK(!mapped_file_->IsValid()); DCHECK(!mapped_file_ || !mapped_file_->IsValid()); https://codereview.chromium.org/1291933002/diff/40001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:194: if (mapped_file_ && !mapped_file_->IsValid()) { !mapped_file_ || !mapped_file_->IsValid() https://codereview.chromium.org/1291933002/diff/40001/media/capture/video/fil... File media/capture/video/file_video_capture_device_factory.cc (right): https://codereview.chromium.org/1291933002/diff/40001/media/capture/video/fil... media/capture/video/file_video_capture_device_factory.cc:68: if (mapped_file && !mapped_file->IsValid()) !mapped_file || !mapped_file->IsValid()
https://codereview.chromium.org/1291933002/diff/40001/media/capture/video/fil... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/40001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:192: DCHECK(!mapped_file_->IsValid()); On 2015/08/17 03:43:34, kcwu wrote: > DCHECK(!mapped_file_ || !mapped_file_->IsValid()); Done. https://codereview.chromium.org/1291933002/diff/40001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:194: if (mapped_file_ && !mapped_file_->IsValid()) { On 2015/08/17 03:43:34, kcwu wrote: > !mapped_file_ || !mapped_file_->IsValid() Done. https://codereview.chromium.org/1291933002/diff/40001/media/capture/video/fil... File media/capture/video/file_video_capture_device_factory.cc (right): https://codereview.chromium.org/1291933002/diff/40001/media/capture/video/fil... media/capture/video/file_video_capture_device_factory.cc:68: if (mapped_file && !mapped_file->IsValid()) On 2015/08/17 03:43:34, kcwu wrote: > !mapped_file || !mapped_file->IsValid() Done.
lgtm % nits https://codereview.chromium.org/1291933002/diff/60001/media/capture/video/fil... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/60001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:19: int ParseY4MInt(const base::StringPiece& token) { Enclose this and other functions in this file into unnamed namespace. https://codereview.chromium.org/1291933002/diff/60001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/60001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:274: const char** eoi_ptr) { please add comment to explain |eoi_ptr|. It is location to end of image (after EOI marker). Not beginning of EOI marker.
henryhsu@chromium.org changed reviewers: + xhwang@chromium.org
xhwang@chromium.org: Please review changes. https://codereview.chromium.org/1291933002/diff/60001/media/capture/video/fil... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/60001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:19: int ParseY4MInt(const base::StringPiece& token) { On 2015/08/17 06:24:13, kcwu wrote: > Enclose this and other functions in this file into unnamed namespace. Done. https://codereview.chromium.org/1291933002/diff/60001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/60001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:274: const char** eoi_ptr) { On 2015/08/17 06:24:13, kcwu wrote: > please add comment to explain |eoi_ptr|. > It is location to end of image (after EOI marker). Not beginning of EOI marker. Done.
https://codereview.chromium.org/1291933002/diff/80001/media/capture/video/fil... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/80001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:17: static const int kMJpegFrameRate = 25; Let's use 30. Camera outputs are usually 30fps. If we want to have jpeg perf test in the future, 30fps is also better.
Hi xhwang, please take a look. This issue should be landed before the branching date August 21th. https://codereview.chromium.org/1291933002/diff/80001/media/capture/video/fil... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/80001/media/capture/video/fil... media/capture/video/file_video_capture_device.cc:17: static const int kMJpegFrameRate = 25; On 2015/08/17 14:06:38, wuchengli wrote: > Let's use 30. Camera outputs are usually 30fps. If we want to have jpeg perf > test in the future, 30fps is also better. Done.
wuchengli@chromium.org changed reviewers: + mcasas@chromium.org - posciak@chromium.org
+mcasas because he's the original author of file_video_capture_device.cc and owner of media/capture/video. mcasas@ Can you take a look at media/capture/video files? Thanks.
https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:100: return video_format->IsValid(); add a local variable and update |video_format| only if valid. https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:136: mapped_file.reset(new base::MemoryMappedFile()); combine with previous line https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:137: if (!mapped_file) no need to check the result of new https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:141: LOG(ERROR) << "File memory map error: " << file_path.value(); return nullptr; https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:232: if (!ParseJpegPicture(mapped_file_->data() + current_byte_index_, buf_ptr https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:239: image_size = frame_size_; Use current_byte_index_ += frame_size_ here to avoid naming confusion. Remove |image_size|. https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... File media/capture/video/file_video_capture_device.h (right): https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:45: // represents the Y4M/MJPEG file to stream repeatedly. Y4M or MJPEG https://codereview.chromium.org/1291933002/diff/100001/media/filters/jpeg_par... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/100001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:273: static bool SearchEOI(const char* buffer, Please make sure this doesn't affect the performance of HW decode. It's not worthwhile for a test to affect the actual performance.
Will update performance data later. https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:100: return video_format->IsValid(); On 2015/08/18 09:41:07, wuchengli wrote: > add a local variable and update |video_format| only if valid. Done. https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:136: mapped_file.reset(new base::MemoryMappedFile()); On 2015/08/18 09:41:07, wuchengli wrote: > combine with previous line Done. https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:137: if (!mapped_file) On 2015/08/18 09:41:07, wuchengli wrote: > no need to check the result of new Done. https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:141: LOG(ERROR) << "File memory map error: " << file_path.value(); On 2015/08/18 09:41:07, wuchengli wrote: > return nullptr; Done. https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:232: if (!ParseJpegPicture(mapped_file_->data() + current_byte_index_, On 2015/08/18 09:41:07, wuchengli wrote: > buf_ptr Done. https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:239: image_size = frame_size_; On 2015/08/18 09:41:07, wuchengli wrote: > Use current_byte_index_ += frame_size_ here to avoid naming confusion. Remove > |image_size|. Done. https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... File media/capture/video/file_video_capture_device.h (right): https://codereview.chromium.org/1291933002/diff/100001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:45: // represents the Y4M/MJPEG file to stream repeatedly. On 2015/08/18 09:41:07, wuchengli wrote: > Y4M or MJPEG Done.
LGTM. Don't forget to let us know the performance data. I think it should be fine. Let's measure to confirm. xhwang: owner review for media/filters. Thanks. mcasas: owner review for media/capture/video. Thanks.
media/ LGTM with nits. https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:272: // |eoi_ptr| is pointing to the end of image (after EOI marker). This comment is ambiguous. I suppose you mean that |eoi_ptr| will point to the end of image after the search succeeds? https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:273: static bool SearchEOI(const char* buffer, What does the return value mean? https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:274: size_t length, Is the indent off by 1 char? Try run "git cl format" to format your CL... https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... media/filters/jpeg_parser.h:41: JPEG_EOI = 0xD9, // end of image Can we order these by value? https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... media/filters/jpeg_parser.h:97: const char* data; Is this for the whole compressed data?
https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:19: static int ParseY4MInt(const base::StringPiece& token) { IIRC, media/ folder has this strange thing of _not_ prepending |static| to file-scoped functions. Elders of this area might know why. https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:99: if (!format.IsValid()) The original CHECK() was correct (and hammered by fischman@ to me in my noogler days), the idea was to crash Chrome if the Y4M input file was not supported in any way. https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:113: std::string header(buf, kY4MHeaderMaxSize - 1); const (maybe) https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:114: size_t header_end = header.find(kY4MSimpleFrameDelimiter); const https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:244: // Play the video repeatly. s/repeatly/repeatedly/ but I'd just remove this comment altogether, or write sth like "rewind the pointer |bla_|" https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... File media/capture/video/file_video_capture_device.h (right): https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:26: // Example Y4M videos can be found in http://media.xiph.org/video/derf. Any example MJPEG files plz? https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:27: class MEDIA_EXPORT FileVideoCaptureDevice : public VideoCaptureDevice { Hmmm, I have the impression that we are mixing two different things. Proposal: Refactor this into three classes: FileVideoCaptureDevice holds the generic parts, and has a |scoped_ptr<VideoFileParser> file_parser_;| member. VideoFileParser is implemented in the .cc file as either MjpegFileParser or Y4MFileParser. FVCD() ctor checks the extension of the file and creates the appropriate parser, using base::EndsWith(file_name.c_str(), suffix, base::CompareCase::INSENSITIVE_ASCII) (bonus points for a VideoFileParser factory method). All header parsing might be stuffed into a bool VideoFileParser::Init(); FVCD uses VFP only real method after that: const uint8_t* VideoFileParser::GetNextFrame(); WDYT? https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:72: scoped_ptr<base::MemoryMappedFile> mapped_file_; A typical Y4M file can be a few Gigabytes long, would MemoryMappedFile handle it correctly?
https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:19: static int ParseY4MInt(const base::StringPiece& token) { On 2015/08/18 18:28:42, mcasas (slow to review) wrote: > IIRC, media/ folder has this strange thing > of _not_ prepending |static| to file-scoped > functions. Elders of this area might know > why. Hi xhwang, do you know this? https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:99: if (!format.IsValid()) On 2015/08/18 18:28:42, mcasas (slow to review) wrote: > The original CHECK() was correct (and hammered by > fischman@ to me in my noogler days), the idea was > to crash Chrome if the Y4M input file was not > supported in any way. Current implementation is to parse Y4M and MJPEG. Hmm...We can crash Chrome with unsupported format if we check the file extension first. https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... File media/capture/video/file_video_capture_device.h (right): https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:26: // Example Y4M videos can be found in http://media.xiph.org/video/derf. On 2015/08/18 18:28:42, mcasas (slow to review) wrote: > Any example MJPEG files plz? media/data/test/bear.mjpeg concatenates many MJPEG images into a file. Therefore, MJPEG stream doesn't describe the frame rate. https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:27: class MEDIA_EXPORT FileVideoCaptureDevice : public VideoCaptureDevice { On 2015/08/18 18:28:42, mcasas (slow to review) wrote: > Hmmm, I have the impression that we are mixing two > different things. > > Proposal: Refactor this into three classes: > > FileVideoCaptureDevice holds the generic parts, > and has a |scoped_ptr<VideoFileParser> file_parser_;| > member. > > VideoFileParser is implemented in the .cc file > as either MjpegFileParser or Y4MFileParser. > > FVCD() ctor checks the extension of the file and > creates the appropriate parser, using > base::EndsWith(file_name.c_str(), suffix, base::CompareCase::INSENSITIVE_ASCII) > (bonus points for a VideoFileParser factory method). > > All header parsing might be stuffed into a > bool VideoFileParser::Init(); > > FVCD uses VFP only real method after that: > > const uint8_t* VideoFileParser::GetNextFrame(); > > WDYT? SGTM. VideoFileParser still uses MemoryMappedFile. https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:72: scoped_ptr<base::MemoryMappedFile> mapped_file_; On 2015/08/18 18:28:42, mcasas (slow to review) wrote: > A typical Y4M file can be a few Gigabytes long, > would MemoryMappedFile handle it correctly? MemoryMapedFile doesn't mention the size limitation. And I also tested crowd720_25frames.y4m which is used in autotest. Could you provide a Y4M file then I can test it? https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:272: // |eoi_ptr| is pointing to the end of image (after EOI marker). On 2015/08/18 17:17:55, xhwang wrote: > This comment is ambiguous. I suppose you mean that |eoi_ptr| will point to the > end of image after the search succeeds? yes. thanks for correction https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:273: static bool SearchEOI(const char* buffer, On 2015/08/18 17:17:55, xhwang wrote: > What does the return value mean? The return value means search EOI succeed or not. I'll update the comment. https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... media/filters/jpeg_parser.h:97: const char* data; On 2015/08/18 17:17:55, xhwang wrote: > Is this for the whole compressed data? Yes. But only for one image.
Patchset #8 (id:140001) has been deleted
PTAL https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:114: size_t header_end = header.find(kY4MSimpleFrameDelimiter); On 2015/08/18 18:28:41, mcasas (slow to review) wrote: > const Done. https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:244: // Play the video repeatly. On 2015/08/18 18:28:42, mcasas (slow to review) wrote: > s/repeatly/repeatedly/ > but I'd just remove this comment altogether, > or write sth like "rewind the pointer |bla_|" Done. https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... File media/capture/video/file_video_capture_device.h (right): https://codereview.chromium.org/1291933002/diff/120001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:72: scoped_ptr<base::MemoryMappedFile> mapped_file_; On 2015/08/19 01:57:24, henryhsu wrote: > On 2015/08/18 18:28:42, mcasas (slow to review) wrote: > > A typical Y4M file can be a few Gigabytes long, > > would MemoryMappedFile handle it correctly? > > MemoryMapedFile doesn't mention the size limitation. > And I also tested crowd720_25frames.y4m which is used in autotest. > Could you provide a Y4M file then I can test it? Y4mFileParser uses original base::File implementation. MjpegFileParser uses MemoryMappedFile. https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:274: size_t length, On 2015/08/18 17:17:55, xhwang wrote: > Is the indent off by 1 char? Try run "git cl format" to format your CL... Done. https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/1291933002/diff/120001/media/filters/jpeg_par... media/filters/jpeg_parser.h:41: JPEG_EOI = 0xD9, // end of image On 2015/08/18 17:17:55, xhwang wrote: > Can we order these by value? Done.
https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:45: std::string header(kY4MHeaderMaxSize, 0); s/0/'\0'/ https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:46: file_->Read(0, &header[0], kY4MHeaderMaxSize - 1); s/kY4MHeaderMaxSize - 1/header.size()/ https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:54: video_frame_.reset(new uint8[frame_size_]); uint8_t https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:65: // case, reset the pointer andd read again. s/andd/and/ https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:152: default: why removed break; above this line? https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:224: if (!file_parser) return file_parser != nullptr; https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... File media/capture/video/file_video_capture_device.h (right): https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:33: // pixel format in |video_format|. Returns true on file parsed successfullly, typo: successfully https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:86: scoped_ptr<uint8[]> video_frame_; uint8_t
Looking good on the FileVCD side https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:18: static const int kMJpegFrameRate = 30; float, 30.0f https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:80: int FileVideoCaptureDevice::Y4mFileParser::ParseY4MInt( This method should be a file function as before. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:89: void FileVideoCaptureDevice::Y4mFileParser::ParseY4MRational( Same: This method should be a file function as before. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:111: void FileVideoCaptureDevice::Y4mFileParser::ParseY4MTags( Same: This method should be a file function as before. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:210: *frame_size = frame_size_ = result.image_size; Should we DCHECK something of |result|? DCHECK_GT(0u, result.data_size); ? https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:212: // reset the pointer to play repeatedly. s/reset/Reset/ https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:223: GetVideoFileParser(file_path, video_format); Creating a VideoFileParser just for checking if the file extension is ".y4m" or ".mjpeg" and parsing its header seems a bit of a waste (we allocate frame-size buffers). Please have another go at it. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:323: CHECK(frame_ptr); and DCHECK(frame_size); ? It might look like overly zealous, but after implementing the first FileVCD I got all kinds of feedback, and most of the complains were easily traceable to strange/erroneous/bizarre files, hence the DCHECK()s everywhere -- so smart developers solve their own problems ISO filing bugs :) https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... File media/capture/video/file_video_capture_device.h (right): https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:69: class Y4mFileParser : public VideoFileParser { Move Y4MFileParser and MjpegFileParser to the implementation file. Actually VideoFileParser can also be moved to the .cc file and just fwd declared here.
https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:18: static const int kMJpegFrameRate = 30; On 2015/08/19 22:17:35, mcasas wrote: > float, 30.0f Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:45: std::string header(kY4MHeaderMaxSize, 0); On 2015/08/19 11:29:38, kcwu wrote: > s/0/'\0'/ Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:46: file_->Read(0, &header[0], kY4MHeaderMaxSize - 1); On 2015/08/19 11:29:38, kcwu wrote: > s/kY4MHeaderMaxSize - 1/header.size()/ Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:54: video_frame_.reset(new uint8[frame_size_]); On 2015/08/19 11:29:38, kcwu wrote: > uint8_t Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:65: // case, reset the pointer andd read again. On 2015/08/19 11:29:38, kcwu wrote: > s/andd/and/ Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:80: int FileVideoCaptureDevice::Y4mFileParser::ParseY4MInt( On 2015/08/19 22:17:34, mcasas wrote: > This method should be a file function as before. Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:89: void FileVideoCaptureDevice::Y4mFileParser::ParseY4MRational( On 2015/08/19 22:17:34, mcasas wrote: > Same: This method should be a file function as before. Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:111: void FileVideoCaptureDevice::Y4mFileParser::ParseY4MTags( On 2015/08/19 22:17:34, mcasas wrote: > Same: This method should be a file function as before. Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:152: default: On 2015/08/19 11:29:38, kcwu wrote: > why removed break; above this line? Thanks. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:210: *frame_size = frame_size_ = result.image_size; On 2015/08/19 22:17:35, mcasas wrote: > Should we DCHECK something of |result|? > DCHECK_GT(0u, result.data_size); ? We don't use result.data_size. I think if ParseJpegPicture returns true, we can use the result directly without check. All error cases will cause ParseJpegPicture return false. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:212: // reset the pointer to play repeatedly. On 2015/08/19 22:17:34, mcasas wrote: > s/reset/Reset/ Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:223: GetVideoFileParser(file_path, video_format); On 2015/08/19 22:17:34, mcasas wrote: > Creating a VideoFileParser just for checking > if the file extension is ".y4m" or ".mjpeg" > and parsing its header seems a bit of a waste > (we allocate frame-size buffers). Please have > another go at it. I move buffer allocation to GetNextFrame. Only allocate the buffer when we need. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:224: if (!file_parser) On 2015/08/19 11:29:38, kcwu wrote: > return file_parser != nullptr; Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:323: CHECK(frame_ptr); On 2015/08/19 22:17:35, mcasas wrote: > and DCHECK(frame_size); ? > > It might look like overly zealous, but after > implementing the first FileVCD I got all kinds > of feedback, and most of the complains were > easily traceable to strange/erroneous/bizarre > files, hence the DCHECK()s everywhere -- so > smart developers solve their own problems ISO > filing bugs :) Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... File media/capture/video/file_video_capture_device.h (right): https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:33: // pixel format in |video_format|. Returns true on file parsed successfullly, On 2015/08/19 11:29:38, kcwu wrote: > typo: successfully Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:69: class Y4mFileParser : public VideoFileParser { On 2015/08/19 22:17:35, mcasas wrote: > Move Y4MFileParser and MjpegFileParser to > the implementation file. > > Actually VideoFileParser can also be > moved to the .cc file and just fwd declared > here. Done. https://codereview.chromium.org/1291933002/diff/160001/media/capture/video/fi... media/capture/video/file_video_capture_device.h:86: scoped_ptr<uint8[]> video_frame_; On 2015/08/19 11:29:38, kcwu wrote: > uint8_t Done.
lgtm
Original performance: cpu idle 83.98%, latency 4.60ms With this patch set 9: cpu idle 81.22%, latency 5.39ms So I added ParseJpegStream function to search EOI when we are using file as fake camera. Real video capture will use ParseJpegPicture function to keep original performance.
https://codereview.chromium.org/1291933002/diff/200001/media/filters/jpeg_par... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/200001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:283: if (marker1 != JPEG_MARKER_PREFIX) Does it help performance if using memcmp here? https://codereview.chromium.org/1291933002/diff/200001/media/filters/jpeg_par... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/1291933002/diff/200001/media/filters/jpeg_par... media/filters/jpeg_parser.h:120: // members, see JPEG specification at How about move the last sentence to the head of this file? Avoid duplication for both functions.
https://codereview.chromium.org/1291933002/diff/200001/media/filters/jpeg_par... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/200001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:283: if (marker1 != JPEG_MARKER_PREFIX) On 2015/08/20 09:01:38, kcwu wrote: > Does it help performance if using memcmp here? I mean memchr.
https://codereview.chromium.org/1291933002/diff/200001/media/filters/jpeg_par... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/200001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:283: if (marker1 != JPEG_MARKER_PREFIX) On 2015/08/20 09:18:36, kcwu wrote: > On 2015/08/20 09:01:38, kcwu wrote: > > Does it help performance if using memcmp here? > > I mean memchr. Yes, it help performance. Thanks. https://codereview.chromium.org/1291933002/diff/200001/media/filters/jpeg_par... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/1291933002/diff/200001/media/filters/jpeg_par... media/filters/jpeg_parser.h:120: // members, see JPEG specification at On 2015/08/20 09:01:38, kcwu wrote: > How about move the last sentence to the head of this file? Avoid duplication for > both functions. Done.
lgtm % nits https://codereview.chromium.org/1291933002/diff/220001/media/filters/jpeg_par... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/220001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:281: const char* marker1_offset = static_cast<const char*>( s/offset/ptr/
https://codereview.chromium.org/1291933002/diff/220001/media/filters/jpeg_par... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/1291933002/diff/220001/media/filters/jpeg_par... media/filters/jpeg_parser.cc:281: const char* marker1_offset = static_cast<const char*>( On 2015/08/20 09:59:09, kcwu wrote: > s/offset/ptr/ Done.
LGTM % nits and 1 comment. https://codereview.chromium.org/1291933002/diff/240001/media/capture/video/fi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/240001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:50: media::VideoCaptureFormat format; I don't see the need to change this function. Please use incoming |video_format|. https://codereview.chromium.org/1291933002/diff/240001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:121: class Y4mFileParser : public VideoFileParser { nit: |final| https://codereview.chromium.org/1291933002/diff/240001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:137: class MjpegFileParser : public VideoFileParser { nit: |final| https://codereview.chromium.org/1291933002/diff/240001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:286: if (!file_parser || !file_parser->Initialize(video_format)) { nit: unnecessary check for !file_parser
https://codereview.chromium.org/1291933002/diff/240001/media/capture/video/fi... File media/capture/video/file_video_capture_device.cc (right): https://codereview.chromium.org/1291933002/diff/240001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:50: media::VideoCaptureFormat format; On 2015/08/20 23:23:54, mcasas wrote: > I don't see the need to change this function. > Please use incoming |video_format|. This is to make sure we only modify |video_format| when ParseY4MTags succeed. https://codereview.chromium.org/1291933002/diff/240001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:121: class Y4mFileParser : public VideoFileParser { On 2015/08/20 23:23:54, mcasas wrote: > nit: |final| Done. https://codereview.chromium.org/1291933002/diff/240001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:137: class MjpegFileParser : public VideoFileParser { On 2015/08/20 23:23:54, mcasas wrote: > nit: |final| Done. https://codereview.chromium.org/1291933002/diff/240001/media/capture/video/fi... media/capture/video/file_video_capture_device.cc:286: if (!file_parser || !file_parser->Initialize(video_format)) { On 2015/08/20 23:23:54, mcasas wrote: > nit: unnecessary check for !file_parser Done.
The CQ bit was checked by henryhsu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wuchengli@chromium.org, xhwang@chromium.org, kcwu@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1291933002/#ps260001 (title: "fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291933002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291933002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by henryhsu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291933002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291933002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by henryhsu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wuchengli@chromium.org, kcwu@chromium.org, xhwang@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1291933002/#ps280001 (title: "fix windows compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291933002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291933002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by henryhsu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wuchengli@chromium.org, kcwu@chromium.org, xhwang@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1291933002/#ps300001 (title: "fix windows compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291933002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291933002/300001
Message was sent while issue was closed.
Committed patchset #15 (id:300001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/cc92a03c8029eda1e443d7ecd1750976d6aa8707 Cr-Commit-Position: refs/heads/master@{#344749} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
