From: Eino-Ville Talvala Date: Tue, 13 Nov 2012 19:45:12 +0000 (-0800) Subject: Camera: Actually make camera_metadata memcopyable X-Git-Tag: android-x86-7.1-r1~533^2~2 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=b10d56ad43a91924d3666127963e5fdce725389c;p=android-x86%2Fsystem-media.git Camera: Actually make camera_metadata memcopyable - Use internal offsets instead of pointers for the entry and data arrays. - Add test to verify memcpy works and doesn't alias data. - Remove support for forward-compatible reserved space handling, given that it's unlikely to be useful Bug: 7546079 Change-Id: I439aa27fed8d243b7a04155daf5e58fa1c4c730e --- diff --git a/camera/src/Android.mk b/camera/src/Android.mk index 19a6f5b9..d3366182 100644 --- a/camera/src/Android.mk +++ b/camera/src/Android.mk @@ -17,6 +17,7 @@ LOCAL_MODULE_TAGS := optional LOCAL_CFLAGS += \ -Wall \ -fvisibility=hidden \ + -std=c99 include $(BUILD_SHARED_LIBRARY) diff --git a/camera/src/camera_metadata.c b/camera/src/camera_metadata.c index c922c7fc..e04b32cd 100644 --- a/camera/src/camera_metadata.c +++ b/camera/src/camera_metadata.c @@ -34,7 +34,7 @@ #define DATA_ALIGNMENT _Alignas(camera_metadata_rational_t) #define ALIGN_TO(val, alignment) \ - (typeof(val))(((uint32_t)(val) + ((alignment) - 1)) & ~((alignment) - 1)) + (((uint32_t)(val) + ((alignment) - 1)) & ~((alignment) - 1)) /** * A single metadata entry, storing an array of values of a given type. If the @@ -93,10 +93,10 @@ struct camera_metadata { uint32_t flags; size_t entry_count; size_t entry_capacity; - camera_metadata_buffer_entry_t *entries; + ptrdiff_t entries_start; // Offset from camera_metadata size_t data_count; size_t data_capacity; - uint8_t *data; + ptrdiff_t data_start; // Offset from camera_metadata void *user; // User set pointer, not copied with buffer uint8_t reserved[0]; }; @@ -134,6 +134,16 @@ const char *camera_metadata_type_names[NUM_TYPES] = { [TYPE_RATIONAL] = "rational" }; +static camera_metadata_buffer_entry_t *get_entries( + const camera_metadata_t *metadata) { + return (camera_metadata_buffer_entry_t*) + ((uint8_t*)metadata + metadata->entries_start); +} + +static uint8_t *get_data(const camera_metadata_t *metadata) { + return (uint8_t*)metadata + metadata->data_start; +} + camera_metadata_t *allocate_camera_metadata(size_t entry_capacity, size_t data_capacity) { if (entry_capacity == 0) return NULL; @@ -162,17 +172,17 @@ camera_metadata_t *place_camera_metadata(void *dst, metadata->flags = 0; metadata->entry_count = 0; metadata->entry_capacity = entry_capacity; - camera_metadata_buffer_entry_t* entries_unaligned = - (camera_metadata_buffer_entry_t*)(metadata + 1); - metadata->entries = ALIGN_TO(entries_unaligned, ENTRY_ALIGNMENT); + metadata->entries_start = + ALIGN_TO(sizeof(camera_metadata_t), ENTRY_ALIGNMENT); metadata->data_count = 0; metadata->data_capacity = data_capacity; metadata->size = memory_needed; if (metadata->data_capacity != 0) { - uint8_t *data_unaligned = (uint8_t*)(metadata->entries + metadata->entry_capacity); - metadata->data = ALIGN_TO(data_unaligned, DATA_ALIGNMENT); + size_t data_unaligned = (uint8_t*)(get_entries(metadata) + + metadata->entry_capacity) - (uint8_t*)metadata; + metadata->data_start = ALIGN_TO(data_unaligned, DATA_ALIGNMENT); } else { - metadata->data = NULL; + metadata->data_start = 0; } metadata->user = NULL; @@ -203,12 +213,8 @@ size_t get_camera_metadata_size(const camera_metadata_t *metadata) { size_t get_camera_metadata_compact_size(const camera_metadata_t *metadata) { if (metadata == NULL) return ERROR; - ptrdiff_t reserved_size = metadata->size - - calculate_camera_metadata_size(metadata->entry_capacity, - metadata->data_capacity); - return calculate_camera_metadata_size(metadata->entry_count, - metadata->data_count) + reserved_size; + metadata->data_count); } size_t get_camera_metadata_entry_count(const camera_metadata_t *metadata) { @@ -234,30 +240,16 @@ camera_metadata_t* copy_camera_metadata(void *dst, size_t dst_size, if (dst == NULL) return NULL; if (dst_size < memory_needed) return NULL; - // If copying a newer version of the structure, there may be additional - // header fields we don't know about but need to copy - ptrdiff_t reserved_size = src->size - - calculate_camera_metadata_size(src->entry_capacity, - src->data_capacity); + camera_metadata_t *metadata = + place_camera_metadata(dst, dst_size, src->entry_count, src->data_count); - camera_metadata_t *metadata = (camera_metadata_t*)dst; - metadata->version = CURRENT_METADATA_VERSION; metadata->flags = src->flags; metadata->entry_count = src->entry_count; - metadata->entry_capacity = src->entry_count; - metadata->entries = (camera_metadata_buffer_entry_t*) - ((uint8_t *)(metadata + 1) + reserved_size); metadata->data_count = src->data_count; - metadata->data_capacity = src->data_count; - metadata->data = (uint8_t *)(metadata->entries + metadata->entry_capacity); - metadata->size = memory_needed; - if (reserved_size > 0) { - memcpy(metadata->reserved, src->reserved, reserved_size); - } - memcpy(metadata->entries, src->entries, + memcpy(get_entries(metadata), get_entries(src), sizeof(camera_metadata_buffer_entry_t[metadata->entry_count])); - memcpy(metadata->data, src->data, + memcpy(get_data(metadata), get_data(src), sizeof(uint8_t[metadata->data_count])); metadata->user = NULL; @@ -271,17 +263,15 @@ int append_camera_metadata(camera_metadata_t *dst, if (dst->entry_capacity < src->entry_count + dst->entry_count) return ERROR; if (dst->data_capacity < src->data_count + dst->data_count) return ERROR; - memcpy(dst->entries + dst->entry_count, src->entries, + memcpy(get_entries(dst) + dst->entry_count, get_entries(src), sizeof(camera_metadata_buffer_entry_t[src->entry_count])); - memcpy(dst->data + dst->data_count, src->data, + memcpy(get_data(dst) + dst->data_count, get_data(src), sizeof(uint8_t[src->data_count])); if (dst->data_count != 0) { - unsigned int i; - for (i = dst->entry_count; - i < dst->entry_count + src->entry_count; - i++) { - camera_metadata_buffer_entry_t *entry = dst->entries + i; - if ( camera_metadata_type_size[entry->type] * entry->count > 4 ) { + camera_metadata_buffer_entry_t *entry = get_entries(dst) + dst->entry_count; + for (size_t i = 0; i < src->entry_count; i++, entry++) { + if ( calculate_camera_metadata_entry_data_size(entry->type, + entry->count) > 0 ) { entry->data.offset += dst->data_count; } } @@ -341,7 +331,7 @@ static int add_camera_metadata_entry_raw(camera_metadata_t *dst, size_t data_payload_bytes = data_count * camera_metadata_type_size[type]; - camera_metadata_buffer_entry_t *entry = dst->entries + dst->entry_count; + camera_metadata_buffer_entry_t *entry = get_entries(dst) + dst->entry_count; entry->tag = tag; entry->type = type; entry->count = data_count; @@ -351,7 +341,7 @@ static int add_camera_metadata_entry_raw(camera_metadata_t *dst, data_payload_bytes); } else { entry->data.offset = dst->data_count; - memcpy(dst->data + entry->data.offset, data, + memcpy(get_data(dst) + entry->data.offset, data, data_payload_bytes); dst->data_count += data_bytes; } @@ -390,7 +380,7 @@ int sort_camera_metadata(camera_metadata_t *dst) { if (dst == NULL) return ERROR; if (dst->flags & FLAG_SORTED) return OK; - qsort(dst->entries, dst->entry_count, + qsort(get_entries(dst), dst->entry_count, sizeof(camera_metadata_buffer_entry_t), compare_entry_tags); dst->flags |= FLAG_SORTED; @@ -404,7 +394,7 @@ int get_camera_metadata_entry(camera_metadata_t *src, if (src == NULL || entry == NULL) return ERROR; if (index >= src->entry_count) return ERROR; - camera_metadata_buffer_entry_t *buffer_entry = src->entries + index; + camera_metadata_buffer_entry_t *buffer_entry = get_entries(src) + index; entry->index = index; entry->tag = buffer_entry->tag; @@ -412,7 +402,7 @@ int get_camera_metadata_entry(camera_metadata_t *src, entry->count = buffer_entry->count; if (buffer_entry->count * camera_metadata_type_size[buffer_entry->type] > 4) { - entry->data.u8 = src->data + buffer_entry->data.offset; + entry->data.u8 = get_data(src) + buffer_entry->data.offset; } else { entry->data.u8 = buffer_entry->data.value; } @@ -431,16 +421,17 @@ int find_camera_metadata_entry(camera_metadata_t *src, camera_metadata_buffer_entry_t key; key.tag = tag; search_entry = bsearch(&key, - src->entries, + get_entries(src), src->entry_count, sizeof(camera_metadata_buffer_entry_t), compare_entry_tags); if (search_entry == NULL) return NOT_FOUND; - index = search_entry - src->entries; + index = search_entry - get_entries(src); } else { // Not sorted, linear search - for (index = 0; index < src->entry_count; index++) { - if (src->entries[index].tag == tag) { + camera_metadata_buffer_entry_t *search_entry = get_entries(src); + for (index = 0; index < src->entry_count; index++, search_entry++) { + if (search_entry->tag == tag) { break; } } @@ -464,19 +455,19 @@ int delete_camera_metadata_entry(camera_metadata_t *dst, if (dst == NULL) return ERROR; if (index >= dst->entry_count) return ERROR; - camera_metadata_buffer_entry_t *entry = dst->entries + index; + camera_metadata_buffer_entry_t *entry = get_entries(dst) + index; size_t data_bytes = calculate_camera_metadata_entry_data_size(entry->type, entry->count); if (data_bytes > 0) { // Shift data buffer to overwrite deleted data - uint8_t *start = dst->data + entry->data.offset; + uint8_t *start = get_data(dst) + entry->data.offset; uint8_t *end = start + data_bytes; size_t length = dst->data_count - entry->data.offset - data_bytes; memmove(start, end, length); // Update all entry indices to account for shift - camera_metadata_buffer_entry_t *e = dst->entries; + camera_metadata_buffer_entry_t *e = get_entries(dst); size_t i; for (i = 0; i < dst->entry_count; i++) { if (calculate_camera_metadata_entry_data_size( @@ -505,7 +496,7 @@ int update_camera_metadata_entry(camera_metadata_t *dst, if (dst == NULL) return ERROR; if (index >= dst->entry_count) return ERROR; - camera_metadata_buffer_entry_t *entry = dst->entries + index; + camera_metadata_buffer_entry_t *entry = get_entries(dst) + index; size_t data_bytes = calculate_camera_metadata_entry_data_size(entry->type, @@ -524,14 +515,14 @@ int update_camera_metadata_entry(camera_metadata_t *dst, } if (entry_bytes != 0) { // Remove old data - uint8_t *start = dst->data + entry->data.offset; + uint8_t *start = get_data(dst) + entry->data.offset; uint8_t *end = start + entry_bytes; size_t length = dst->data_count - entry->data.offset - entry_bytes; memmove(start, end, length); dst->data_count -= entry_bytes; // Update all entry indices to account for shift - camera_metadata_buffer_entry_t *e = dst->entries; + camera_metadata_buffer_entry_t *e = get_entries(dst); size_t i; for (i = 0; i < dst->entry_count; i++) { if (calculate_camera_metadata_entry_data_size( @@ -547,12 +538,12 @@ int update_camera_metadata_entry(camera_metadata_t *dst, // Append new data entry->data.offset = dst->data_count; - memcpy(dst->data + entry->data.offset, data, data_payload_bytes); + memcpy(get_data(dst) + entry->data.offset, data, data_payload_bytes); dst->data_count += data_bytes; } } else if (data_bytes != 0) { // data size unchanged, reuse same data location - memcpy(dst->data + entry->data.offset, data, data_payload_bytes); + memcpy(get_data(dst) + entry->data.offset, data, data_payload_bytes); } if (data_bytes == 0) { @@ -661,8 +652,8 @@ void dump_indented_camera_metadata(const camera_metadata_t *metadata, fdprintf(fd, "%*sVersion: %d, Flags: %08x\n", indentation + 2, "", metadata->version, metadata->flags); - for (i=0; i < metadata->entry_count; i++) { - camera_metadata_buffer_entry_t *entry = metadata->entries + i; + camera_metadata_buffer_entry_t *entry = get_entries(metadata); + for (i=0; i < metadata->entry_count; i++, entry++) { const char *tag_name, *tag_section; tag_section = get_camera_metadata_section_name(entry->tag); @@ -701,7 +692,7 @@ void dump_indented_camera_metadata(const camera_metadata_t *metadata, metadata->data_count); continue; } - data_ptr = metadata->data + entry->data.offset; + data_ptr = get_data(metadata) + entry->data.offset; } else { data_ptr = entry->data.value; } diff --git a/camera/tests/camera_metadata_tests.cpp b/camera/tests/camera_metadata_tests.cpp index 48f3426b..99bfb451 100644 --- a/camera/tests/camera_metadata_tests.cpp +++ b/camera/tests/camera_metadata_tests.cpp @@ -1616,3 +1616,99 @@ TEST(camera_metadata, user_pointer) { free(buf); free_camera_metadata(m); } + +TEST(camera_metadata, memcpy) { + camera_metadata_t *m = NULL; + const size_t entry_capacity = 50; + const size_t data_capacity = 450; + + int result; + + m = allocate_camera_metadata(entry_capacity, data_capacity); + + add_test_metadata(m, 5); + + uint8_t *dst = new uint8_t[get_camera_metadata_size(m)]; + + memcpy(dst, m, get_camera_metadata_size(m)); + + camera_metadata_t *m2 = reinterpret_cast(dst); + + ASSERT_EQ(get_camera_metadata_size(m), + get_camera_metadata_size(m2)); + EXPECT_EQ(get_camera_metadata_compact_size(m), + get_camera_metadata_compact_size(m2)); + ASSERT_EQ(get_camera_metadata_entry_count(m), + get_camera_metadata_entry_count(m2)); + EXPECT_EQ(get_camera_metadata_entry_capacity(m), + get_camera_metadata_entry_capacity(m2)); + EXPECT_EQ(get_camera_metadata_data_count(m), + get_camera_metadata_data_count(m2)); + EXPECT_EQ(get_camera_metadata_data_capacity(m), + get_camera_metadata_data_capacity(m2)); + + camera_metadata_entry_t e1, e2; + for (size_t i = 0; i < get_camera_metadata_entry_count(m); i++) { + result = get_camera_metadata_entry(m, i, &e1); + ASSERT_EQ(OK, result); + result = get_camera_metadata_entry(m2, i, &e2); + ASSERT_EQ(OK, result); + + EXPECT_EQ(e1.index, e2.index); + EXPECT_EQ(e1.tag, e2.tag); + ASSERT_EQ(e1.type, e2.type); + ASSERT_EQ(e1.count, e2.count); + + ASSERT_TRUE(!memcmp(e1.data.u8, e2.data.u8, + camera_metadata_type_size[e1.type] * e1.count)); + } + + // Make sure updating one metadata buffer doesn't change the other + + int64_t double_exposure_time[] = { 100, 200 }; + + result = update_camera_metadata_entry(m, 0, + double_exposure_time, + sizeof(double_exposure_time)/sizeof(int64_t), NULL); + EXPECT_EQ(OK, result); + + result = get_camera_metadata_entry(m, 0, &e1); + ASSERT_EQ(OK, result); + result = get_camera_metadata_entry(m2, 0, &e2); + ASSERT_EQ(OK, result); + + EXPECT_EQ(e1.index, e2.index); + EXPECT_EQ(e1.tag, e2.tag); + ASSERT_EQ(e1.type, e2.type); + ASSERT_EQ((size_t)2, e1.count); + ASSERT_EQ((size_t)1, e2.count); + EXPECT_EQ(100, e1.data.i64[0]); + EXPECT_EQ(200, e1.data.i64[1]); + EXPECT_EQ(100, e2.data.i64[0]); + + // And in the reverse direction as well + + double_exposure_time[0] = 300; + result = update_camera_metadata_entry(m2, 0, + double_exposure_time, + sizeof(double_exposure_time)/sizeof(int64_t), NULL); + EXPECT_EQ(OK, result); + + result = get_camera_metadata_entry(m, 0, &e1); + ASSERT_EQ(OK, result); + result = get_camera_metadata_entry(m2, 0, &e2); + ASSERT_EQ(OK, result); + + EXPECT_EQ(e1.index, e2.index); + EXPECT_EQ(e1.tag, e2.tag); + ASSERT_EQ(e1.type, e2.type); + ASSERT_EQ((size_t)2, e1.count); + ASSERT_EQ((size_t)2, e2.count); + EXPECT_EQ(100, e1.data.i64[0]); + EXPECT_EQ(200, e1.data.i64[1]); + EXPECT_EQ(300, e2.data.i64[0]); + EXPECT_EQ(200, e2.data.i64[1]); + + delete dst; + free_camera_metadata(m); +}